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 6, 2021
1 parent 6bab432 commit f8f3f18
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 8 deletions.
66 changes: 59 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,22 @@ 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"},
{"ignore_comments", "true",
"Ignore comments to exceed the limit."},
},
};
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 +110,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 ignore_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 +121,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 ignore_comments_;
default:
return true;
}
Expand Down Expand Up @@ -158,16 +166,58 @@ static bool AllowLongLineException(TokenSequence::const_iterator token_begin,
return false;
}

static verible::TokenRange StripFrontNewlineTokens(
verible::TokenRange token_range) {
// skip prepending newline tokens and return a new, possibly shrinked range
auto new_begin = token_range.begin();
while (new_begin < token_range.end() &&
new_begin->token_enum() == TK_NEWLINE) {
++new_begin;
}
return make_range(new_begin, token_range.end());
}

static verible::TokenRange StripBackNewlineTokens(
verible::TokenRange token_range) {
// skip prepending newline tokens and return a new, possibly shrinked range
auto new_end = token_range.end();
while (new_end > token_range.begin() &&
(new_end - 1)->token_enum() == TK_NEWLINE) {
--new_end;
}
return make_range(token_range.begin(), new_end);
}

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);
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 newline tokens in front/back
// but doesn't remove newline tokens from inside.
token_range = StripFrontNewlineTokens(token_range);
token_range = StripBackNewlineTokens(token_range);
if (token_range.begin() == token_range.end()) {
// No tokens, even though the line exceeds the limit.
// This might mean that there's a token that spans accross multiple
// lines. Use the last non-newline token as a new beginning.
auto new_begin = token_range.begin() - 1;
while (new_begin->token_enum() == TK_NEWLINE &&
new_begin > text_begin) {
--new_begin;
}
// the front was moved backwards, so now it's possible that there are
// unneeded tokens in the back
token_range =
StripBackNewlineTokens(make_range(new_begin, token_range.end()));
}
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 +231,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)},
{"ignore_comments", SetBool(&ignore_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 ignore_comments_ = true;
// 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("ignore_comments:true")).ok());
EXPECT_TRUE((status = rule.Configure("ignore_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("ignore_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;ignore_comments:false");
}

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;ignore_comments:false");
}

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

0 comments on commit f8f3f18

Please sign in to comment.