Skip to content

Commit

Permalink
add line length rule parameter to lint comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@wsipak
wsipak committed Oct 8, 2021
1 parent 43375d7 commit 0275f8d
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 8 deletions.
74 changes: 67 additions & 7 deletions verilog/analysis/checkers/line_length_rule.cc
View file
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", "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.
Expand All @@ -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.
Expand All @@ -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;
}
Expand Down Expand Up @@ -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_));
Expand All @@ -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 {
Expand Down
4 changes: 3 additions & 1 deletion verilog/analysis/checkers/line_length_rule.h
View file
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 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<verible::LintViolation> violations_;
};
Expand Down
34 changes: 34 additions & 0 deletions verilog/analysis/checkers/line_length_rule_test.cc
View file
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

0 comments on commit 0275f8d

Please sign in to comment.