Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

line length rule: add a parameter to check comments #972

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

line length rule: add a parameter to check comments #972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
360d2b8
add line length rule parameter to lint comments
wsipak Oct 1, 2021
793a9be
line_length_rule: Changed the default state of check_comments
jbylicki May 5, 2023
cc4cfdc
line_length_rule: Moved trailing newline check to a new method
jbylicki May 5, 2023
3bd46d0
line_length_rule: Added deductions for reverse_iterator
jbylicki May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 78 additions & 9 deletions verilog/analysis/checkers/line_length_rule.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,21 @@ const LintRuleDescriptor& LineLengthRule::GetDescriptor() {
.desc =
"Checks that all lines do not exceed the maximum allowed "
"length. ",
.param = {{"length", absl::StrCat(kDefaultLineLength),
"Desired line length"}},
.param =
{
{"length", absl::StrCat(kDefaultLineLength),
"Desired line length"},
{"check_comments", "true", "Check comments."},
},
};
return d;
}

// Returns true if line is an exceptional case that should allow excessive
// length.
static bool AllowLongLineException(TokenSequence::const_iterator token_begin,
TokenSequence::const_iterator token_end) {
bool LineLengthRule::AllowLongLineException(
TokenSequence::const_iterator token_begin,
TokenSequence::const_iterator token_end) {
// There may be no tokens on this line if the lexer skipped them.
// TODO(b/134180314): Preserve all text in lexer.
if (token_begin == token_end) return true; // Conservatively ignore.
Expand Down Expand Up @@ -111,7 +116,7 @@ static bool AllowLongLineException(TokenSequence::const_iterator token_begin,
// remain as atomic tokens, so fixing comment indentation may cause
// line-length violations. The compromise for now is to forgive
// this case (no matter what the length).
return true;
return !check_comments_;

// Once comment-reflowing is implemented, re-enable the following:
// If comment consist of more than one token, it should be split.
Expand All @@ -122,6 +127,8 @@ static bool AllowLongLineException(TokenSequence::const_iterator token_begin,
// TODO(fangism): examine "long string literals"
// TODO(fangism): case TK_COMMENT_BLOCK:
// Multi-line comments need deeper inspection.
case TK_COMMENT_BLOCK:
return !check_comments_;
default:
return true;
}
Expand Down Expand Up @@ -165,15 +172,75 @@ static bool AllowLongLineException(TokenSequence::const_iterator token_begin,
return false;
}

static verible::TokenRange StripNewlineTokens(verible::TokenRange token_range) {
// truncate newline tokens and return a new, possibly shrinked range
auto reversed = reversed_view(token_range);
auto last_non_newline =
std::find_if(reversed.begin(), reversed.end(), [](const TokenInfo& t) {
return t.token_enum() != TK_NEWLINE;
}).base();

return make_range(token_range.begin(), last_non_newline);
}

static verible::TokenRange SeekLastNonNewlineToken(
verible::TokenRange token_range, TokenSequence::const_iterator text_begin) {
// Extend token_range backwards to find non-newline tokens.
// text_begin is the frontal limiter that shall not be overrun.
// If such a token isn't found, the input range is returned
auto found_it = std::find_if(
std::reverse_iterator<TokenSequence::const_iterator>(token_range.begin()),
std::reverse_iterator<TokenSequence::const_iterator>(text_begin),
[](const TokenInfo& t) { return t.token_enum() != TK_NEWLINE; });

if (found_it !=
std::reverse_iterator<TokenSequence::const_iterator>(text_begin)) {
Comment on lines +192 to +197
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need explicit params for the reverse_iterators here? (possibly use cbegin() if you're running into some fun template errors)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cbegin is not implemented and not having parameters has caused problems (and warnings in a compilation with -Werror set) with parameter deduction in Clang:

| verilog/analysis/checkers/line_length_rule.cc:192:7: error: 'reverse_iterator' may not intend to support class template argument deduction [-Werror,-Wctad-maybe-unsupported]
12:23:05 |       std::reverse_iterator(token_range.begin()),
12:23:05 |       ^
12:23:05 | /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_iterator.h:132:11: note: add a deduction guide to suppress this warning
12:23:05 |     class reverse_iterator
12:23:05 |           ^
12:23:05 | verilog/analysis/checkers/line_length_rule.cc:193:7: error: 'reverse_iterator' may not intend to support class template argument deduction [-Werror,-Wctad-maybe-unsupported]
12:23:05 |       std::reverse_iterator(text_begin),
12:23:05 |       ^
12:23:05 | /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_iterator.h:132:11: note: add a deduction guide to suppress this warning
12:23:05 |     class reverse_iterator
12:23:05 |           ^
12:23:05 | verilog/analysis/checkers/line_length_rule.cc:196:19: error: 'reverse_iterator' may not intend to support class template argument deduction [-Werror,-Wctad-maybe-unsupported]
12:23:05 |   if (found_it != std::reverse_iterator(text_begin)) {
12:23:05 |                   ^
12:23:05 | /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_iterator.h:132:11: note: add a deduction guide to suppress this warning
12:23:05 |     class reverse_iterator

// Use the last non-newline token as a new beginning.
auto new_begin = found_it.base() - 1;
// We move the front backwards, so now it's possible that there are
// unneeded tokens in the back.
token_range = make_range(new_begin, token_range.end());
}
return token_range;
}

verible::TokenRange& CheckTokenRangeForTrailingNewlines(
verible::TokenRange* orig_range,
TokenSequence::const_iterator text_structure) {
// Recall that token_range is *unfiltered* and may contain non-essential
// whitespace 'tokens'.
// This shrinks the range if there are leading newline tokens
*orig_range = StripNewlineTokens(*orig_range);
if (orig_range->begin() == orig_range->end()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (orig_range->begin() == orig_range->end()) {
if (!std::empty(*orig_range)) return *orig_range;

Maybe something like this? Then just return StripNewlineTokens(...) in the other branch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not implement enough methods to call either std::empty or TokenRange.empty() and your proposal (along with variations I have tried) does not compile. The implementation is at common/util/iterator_range.h.

// No tokens, even though the line exceeds the limit.
// The range doesn't contain tokens that start in the preceeding lines
// and continue in this line.
// A token that spans accross multiple lines is in this line
// and exceeds the limit.
// Find the last non-newline token in the preceeding lines.
*orig_range = SeekLastNonNewlineToken(*orig_range, text_structure);
// Yet again after moving the `begin` backwards, `end` may point
// behind newline tokens, depending on how many lines backwards
// the range was extended.
// Also, AllowLongLineException function assumes the length of the range
// to be 1 if there is 1 meaningful token.
*orig_range = StripNewlineTokens(*orig_range);
}
return *orig_range;
}

void LineLengthRule::Lint(const TextStructureView& text_structure,
absl::string_view) {
size_t lineno = 0;
const auto text_begin = text_structure.TokenRangeOnLine(lineno).begin();

for (const auto& line : text_structure.Lines()) {
const int observed_line_length = verible::utf8_len(line);
if (observed_line_length > line_length_limit_) {
const auto token_range = text_structure.TokenRangeOnLine(lineno);
// Recall that token_range is *unfiltered* and may contain non-essential
// whitespace 'tokens'.
// range of tokens that *begin* in this line.
auto token_range = text_structure.TokenRangeOnLine(lineno);
token_range =
CheckTokenRangeForTrailingNewlines(&token_range, text_begin);
if (!AllowLongLineException(token_range.begin(), token_range.end())) {
// Fake a token that marks the offending range of text.
TokenInfo token(TK_OTHER, line.substr(line_length_limit_));
Expand All @@ -187,10 +254,12 @@ void LineLengthRule::Lint(const TextStructureView& text_structure,
}

absl::Status LineLengthRule::Configure(absl::string_view configuration) {
using verible::config::SetBool;
using verible::config::SetInt;
return verible::ParseNameValues(
configuration, {{"length", SetInt(&line_length_limit_, kMinimumLineLength,
kMaximumLineLength)}});
kMaximumLineLength)},
{"check_comments", SetBool(&check_comments_)}});
}

LintRuleStatus LineLengthRule::Report() const {
Expand Down
4 changes: 3 additions & 1 deletion verilog/analysis/checkers/line_length_rule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,10 @@ class LineLengthRule : public verible::TextStructureLintRule {
verible::LintRuleStatus Report() const final;

private:
bool AllowLongLineException(verible::TokenSequence::const_iterator,
verible::TokenSequence::const_iterator);
int line_length_limit_ = kDefaultLineLength;

bool check_comments_ = false;
// Collection of found violations.
std::set<verible::LintViolation> violations_;
};
Expand Down
34 changes: 34 additions & 0 deletions verilog/analysis/checkers/line_length_rule_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ TEST(LineLengthRuleTest, Configuration) {
absl::Status status;
EXPECT_TRUE((status = rule.Configure("")).ok()) << status.message();
EXPECT_TRUE((status = rule.Configure("length:50")).ok()) << status.message();
EXPECT_TRUE((status = rule.Configure("check_comments:true")).ok());
EXPECT_TRUE((status = rule.Configure("check_comments:false")).ok());

EXPECT_FALSE((status = rule.Configure("foo:42")).ok());
EXPECT_TRUE(absl::StrContains(status.message(), "supported parameter"));
Expand All @@ -48,6 +50,10 @@ TEST(LineLengthRuleTest, Configuration) {

EXPECT_FALSE((status = rule.Configure("length:-1")).ok());
EXPECT_TRUE(absl::StrContains(status.message(), "out of range"));

EXPECT_FALSE((status = rule.Configure("check_comments:zx")).ok());
EXPECT_TRUE(
absl::StrContains(status.message(), "Boolean value should be one of"));
}

// Tests that space-only text passes.
Expand Down Expand Up @@ -199,6 +205,34 @@ TEST(LineLengthRuleTest, RejectsTextConfigured) {
"length:40");
}

TEST(LineLengthRuleTest, RejectsTextComments) {
const std::initializer_list<LintTestCase> kTestCases = {
{"// aaaaaaaaaabbbbbbbbbbcccc cccccdddddds", {TK_OTHER, "X"}, "\n"},
{" //aaaaaaaaaaaabbbbbbbbbbcccccccccddds", {TK_OTHER, "X"}, "\n"},
{" // //shorter comment not exceeding 40\n"}};
RunConfiguredLintTestCases<VerilogAnalyzer, LineLengthRule>(
kTestCases, "length:40;check_comments:true");
}

TEST(LineLengthRuleTest, RejectsTextBlockComments) {
const std::initializer_list<LintTestCase> kTestCases = {
{"/*short comment not exceeding 40 */\n"},
{"/* this block exceeds 40 c", {TK_OTHER, "X"}, "\n*/"},
{"/* this multi line block starts ok \n"
" but this line exceeds **the limit** #",
{TK_OTHER, "X"},
"\n"
" end of the comment */\n"},
{"/* test the last line\n"
" zzzzzzzzzzzzzzzzz\n"
" zzzz zz \n"
"this last line is too long .............",
{TK_OTHER, "X*/"},
"\n"}};
RunConfiguredLintTestCases<VerilogAnalyzer, LineLengthRule>(
kTestCases, "length:40;check_comments:true");
}

#if 0
TEST(LineLengthRuleTest, Encrypted) {
const LintTestCase kTestCases[] = {
Expand Down