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

wsipak · 2021-10-06T14:30:34Z

This addresses #941
There's a new parameter ~~allow_comments~~ ignore_comments.
When it's set to false, exceeding the line length limit in comments won't be forgiven.
The default value is true, so that it doesn't change the way it works by default now.
It works with single line comments and block comments, but those are not distinguished in configuration right now (Should we consider them separately?).

Also, would it be preferred to rename the parameter to something like check_comments so that it has the opposite meaning, and set to false would result in not linting comments?

Would it be better to have two separate values of length limit for non-comment and comment lines?
This probably could be addressed and implemented in another PR, but I'm noting it here as a thing to consider.

codecov-commenter · 2021-10-06T15:15:50Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (de4211b) 93.13% compared to head (5ee7eeb) 93.14%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #972   +/-   ##
=======================================
  Coverage   93.13%   93.14%           
=======================================
  Files         355      355           
  Lines       25843    25882   +39     
=======================================
+ Hits        24069    24107   +38     
- Misses       1774     1775    +1

Impacted Files	Coverage Δ
verilog/analysis/checkers/line_length_rule.h	`100.00% <ø> (ø)`
verilog/analysis/checkers/line_length_rule.cc	`97.33% <100.00%> (-0.63%)`	⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

verilog/analysis/checkers/line_length_rule.cc

hzeller · 2021-10-06T20:04:40Z

Yes, I think for the user's point of view a parameter check_comments would be easiest to understand.

I tihnk diffentiating between end-of-line and block comments w.r.t warning or not is probably too much user-confusing option than worth it (and I wouldn't see why it should make a difference).

Same for different length of comment vs. non-comment. All of them should stay within the same line length.

wsipak · 2021-10-07T13:41:52Z

@hzeller Thank you for your feedback. The parameter is now named check_comments.

hzeller · 2021-10-07T16:40:14Z

verilog/analysis/checkers/line_length_rule.cc

      // 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


Can you explain why we need this newline checking at all ? Aren't we going through line by line anyway, so we wouldn't see the newlines ? Or is it because block comments come as one big whitespace token, possibly spanning multiple lines ?

The intention here needs to be explained in a comment, and possibly the block here put into a well-named function.

I've moved the part which extends token range backwards, seeking for meaningful tokens, to a new function.
Explaining this made me realize that previously there were some excessive operations for shrinking the range, I've fixed it to do what's actually needed.
I've added comments about what the functions do, but I'll also leave some information here with an example.
The rule analyzes text line by line. The function TokenRangeOnLine which is used to retrieve tokens for that line, returns the tokens that begin on this line, not the tokens that begin somewhere earlier and continue in that line. As a result, in order to handle a long line of a token that started somewhere in preceding lines, we need to move the begin iterator backwards.

Like in this example:

/* this is a block comment some text some text some text some very long line, let's assume the limit is 40 characters */

For the long line (6th), the only token retrieved by TokenRangeOnLine would be a newline,
because TK_COMMENT_BLOCK belongs to the 1st line (where the comment block begins).
After extending the range backwards searching for a meaningful token (to the 1st line),
even more newline tokens would be present in the back of the range.
So, when processing the 6th line, the code seeks back to the 1st line (TK_COMMENT_BLOCK is found)
and truncates the range to skip all the newlines, then AllowLongLineException is called with that single token.

hzeller

I think this looks good, but we should probably move the range extension thing into its separate function, which then can 'house' all the necessary explanations why it is needed, but at the actual call-site, it is compact to read.

hzeller · 2021-10-19T04:38:55Z

verilog/analysis/checkers/line_length_rule.cc

+  return make_range(token_range.begin(), last_non_newline);
+}
+
+static verible::TokenRange SeekTokensBackwards(


this should probably be named to what it does: SeekLastNonNewlineToken() or something

Good idea, changed it

hzeller · 2021-10-19T04:43:44Z

verilog/analysis/checkers/line_length_rule.cc

      // 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()) {


Maybe it makes sense to put this in a separate function with a good name that describes what this token-range adjustment is doing.
That makes this code here compact and allows for the necessary explanation inside the function that deals just with this part.

hzeller

What happened to this PR, looks like we were close ?
@wsipak do you still have the branch to finish ?

hzeller · 2023-04-24T18:01:32Z

verilog/analysis/checkers/line_length_rule.cc

+          {
+              {"length", absl::StrCat(kDefaultLineLength),
+               "Desired line length"},
+              {"check_comments", "false", "Check comments."},


Did we previously check the length in comments ? Then maybe this should default to 'true'.

Signed-off-by: Jan Bylicki <[email protected]>

Clang had issued deducing the arguments for reverse_iterator, so typing info had to be added Signed-off-by: Jan Bylicki <[email protected]>

kbieganski · 2023-06-30T07:56:54Z

verilog/analysis/checkers/line_length_rule.cc

+      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)) {


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

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

kbieganski · 2023-06-30T08:06:07Z

verilog/analysis/checkers/line_length_rule.cc

+  // whitespace 'tokens'.
+  // This shrinks the range if there are leading newline tokens
+  *orig_range = StripNewlineTokens(*orig_range);
+  if (orig_range->begin() == orig_range->end()) {


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.

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.

wsipak force-pushed the wsip/line_length branch 2 times, most recently from f8f3f18 to 6a3cbee Compare October 6, 2021 15:27

hzeller reviewed Oct 6, 2021

View reviewed changes

verilog/analysis/checkers/line_length_rule.cc Outdated Show resolved Hide resolved

verilog/analysis/checkers/line_length_rule.cc Outdated Show resolved Hide resolved

wsipak force-pushed the wsip/line_length branch from 6a3cbee to 544cc8c Compare October 7, 2021 12:38

wsipak requested a review from hzeller October 7, 2021 13:50

hzeller reviewed Oct 7, 2021

View reviewed changes

wsipak force-pushed the wsip/line_length branch from 544cc8c to 0275f8d Compare October 8, 2021 14:04

hzeller requested changes Oct 19, 2021

View reviewed changes

hzeller reviewed Apr 24, 2023

View reviewed changes

jbylicki force-pushed the wsip/line_length branch 5 times, most recently from e40aef7 to 5ee7eeb Compare May 5, 2023 12:40

jbylicki requested a review from hzeller May 5, 2023 12:58

wsipak and others added 4 commits June 26, 2023 10:58

add line length rule parameter to lint comments

360d2b8

line_length_rule: Changed the default state of check_comments

793a9be

Signed-off-by: Jan Bylicki <[email protected]>

line_length_rule: Moved trailing newline check to a new method

cc4cfdc

Signed-off-by: Jan Bylicki <[email protected]>

line_length_rule: Added deductions for reverse_iterator

3bd46d0

Clang had issued deducing the arguments for reverse_iterator, so typing info had to be added Signed-off-by: Jan Bylicki <[email protected]>

jbylicki force-pushed the wsip/line_length branch from 5ee7eeb to 3bd46d0 Compare June 26, 2023 08:58

kbieganski requested changes Jun 30, 2023

View reviewed changes

kbieganski approved these changes Jul 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wsipak commented Oct 6, 2021 •

edited

Loading

codecov-commenter commented Oct 6, 2021 •

edited

Loading

hzeller commented Oct 6, 2021 •

edited

Loading

wsipak commented Oct 7, 2021

hzeller Oct 7, 2021

wsipak Oct 8, 2021

hzeller left a comment

hzeller Oct 19, 2021

jbylicki May 5, 2023

hzeller Oct 19, 2021

jbylicki May 5, 2023

hzeller left a comment

hzeller Apr 24, 2023

jbylicki May 5, 2023

kbieganski Jun 30, 2023

jbylicki Jul 3, 2023

kbieganski Jun 30, 2023

jbylicki Jul 3, 2023

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

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

Are you sure you want to change the base?

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

Conversation

wsipak commented Oct 6, 2021 • edited Loading

codecov-commenter commented Oct 6, 2021 • edited Loading

Codecov Report

hzeller commented Oct 6, 2021 • edited Loading

wsipak commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsipak commented Oct 6, 2021 •

edited

Loading

codecov-commenter commented Oct 6, 2021 •

edited

Loading

hzeller commented Oct 6, 2021 •

edited

Loading