From 0275f8d1a0ab72517ea4a5878d5c25f3aa64e735 Mon Sep 17 00:00:00 2001 From: Wojciech Sipak Date: Fri, 1 Oct 2021 11:32:57 +0200 Subject: [PATCH] add line length rule parameter to lint comments --- verilog/analysis/checkers/line_length_rule.cc | 74 +++++++++++++++++-- verilog/analysis/checkers/line_length_rule.h | 4 +- .../checkers/line_length_rule_test.cc | 34 +++++++++ 3 files changed, 104 insertions(+), 8 deletions(-) diff --git a/verilog/analysis/checkers/line_length_rule.cc b/verilog/analysis/checkers/line_length_rule.cc index 25e194a978..f5034d7627 100644 --- a/verilog/analysis/checkers/line_length_rule.cc +++ b/verilog/analysis/checkers/line_length_rule.cc @@ -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", "false", "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. @@ -104,7 +109,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. @@ -115,6 +120,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; } @@ -158,16 +165,67 @@ 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 SeekTokensBackwards( + 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(token_range.begin()), + std::reverse_iterator(text_begin), + [](const TokenInfo& t) { return t.token_enum() != TK_NEWLINE; }); + + if (found_it != std::reverse_iterator(text_begin)) { + // 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; +} + 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()) { VLOG(2) << "Examining line: " << lineno + 1; const int observed_line_length = verible::utf8_len(line); if (observed_line_length > line_length_limit_) { - const auto token_range = text_structure.TokenRangeOnLine(lineno); + // range of tokens that *begin* in this line. + auto token_range = text_structure.TokenRangeOnLine(lineno); // Recall that token_range is *unfiltered* and may contain non-essential // whitespace 'tokens'. + // This shrinks the range if there are leading newline tokens + token_range = StripNewlineTokens(token_range); + if (token_range.begin() == token_range.end()) { + // 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. + token_range = SeekTokensBackwards(token_range, text_begin); + // 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. + token_range = StripNewlineTokens(token_range); + } 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_)); @@ -181,10 +239,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 { diff --git a/verilog/analysis/checkers/line_length_rule.h b/verilog/analysis/checkers/line_length_rule.h index d052a12bbb..40a0a1fbcc 100644 --- a/verilog/analysis/checkers/line_length_rule.h +++ b/verilog/analysis/checkers/line_length_rule.h @@ -54,8 +54,10 @@ class LineLengthRule : public verible::TextStructureLintRule { verible::LintRuleStatus Report() const override; 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 violations_; }; diff --git a/verilog/analysis/checkers/line_length_rule_test.cc b/verilog/analysis/checkers/line_length_rule_test.cc index 9972ec4698..14bfc43dc3 100644 --- a/verilog/analysis/checkers/line_length_rule_test.cc +++ b/verilog/analysis/checkers/line_length_rule_test.cc @@ -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")); @@ -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. @@ -199,6 +205,34 @@ TEST(LineLengthRuleTest, RejectsTextConfigured) { "length:40"); } +TEST(LineLengthRuleTest, RejectsTextComments) { + const std::initializer_list kTestCases = { + {"// aaaaaaaaaabbbbbbbbbbcccc cccccdddddds", {TK_OTHER, "X"}, "\n"}, + {" //aaaaaaaaaaaabbbbbbbbbbcccccccccddds", {TK_OTHER, "X"}, "\n"}, + {" // //shorter comment not exceeding 40\n"}}; + RunConfiguredLintTestCases( + kTestCases, "length:40;check_comments:true"); +} + +TEST(LineLengthRuleTest, RejectsTextBlockComments) { + const std::initializer_list 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( + kTestCases, "length:40;check_comments:true"); +} + #if 0 TEST(LineLengthRuleTest, Encrypted) { const LintTestCase kTestCases[] = {