Page MenuHomePhabricator

[clang-tidy] Add options to bugprone-argument-comment to add missing argument comments to literals
ClosedPublic

Authored by MyDeveloperDay on Feb 4 2019, 2:13 AM.

Details

Summary

bugprone-argument-comment only supports identifying those comments which do not match the function parameter name

This revision add 3 options to adding missing argument comments to literals (granularity on type is added to control verbosity of fixit)

CheckOptions:
  - key:             bugprone-argument-comment.CommentBoolLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentFloatLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentIntegerLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentStringLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentCharacterLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentUserDefinedLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentNullPtrs
    value:           '1'

After applying these options, literal arguments will be preceded with /*ParameterName=*/

Diff Detail

Repository
rL LLVM

Event Timeline

MyDeveloperDay created this revision.Feb 4 2019, 2:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 2:13 AM
Herald added a subscriber: xazax.hun. · View Herald Transcript
MyDeveloperDay marked an inline comment as done.Feb 4 2019, 2:15 AM
MyDeveloperDay added inline comments.
docs/ReleaseNotes.rst
89 ↗(On Diff #185008)

note to self, I will reorganize this alphabetically, on next revision

Fix release note alphabetic order
Add examples of Fixit into the documentation

Eugene.Zelenko added inline comments.Feb 4 2019, 11:33 AM
docs/ReleaseNotes.rst
82 ↗(On Diff #185014)

Please move changes in existing checks after list of new checks.

Can you drop the file mode changes that are in this review?

clang-tidy/bugprone/ArgumentCommentCheck.cpp
292–293 ↗(On Diff #185014)

These can be combined into a single if statement.

300–301 ↗(On Diff #185014)

Why is this split into two lines?

304 ↗(On Diff #185014)

This continue can be dropped without changing the semantics, correct?

clang-tidy/bugprone/ArgumentCommentCheck.h
44–46 ↗(On Diff #185014)

Why not character or string literals?
What about nullptr literals or UDLs?

docs/clang-tidy/checks/bugprone-argument-comment.rst
40 ↗(On Diff #185014)

Format the code examples from our style guide as well (same below).

MyDeveloperDay edited the summary of this revision. (Show Details)

Address review comments
Add support for additional StringLiterals,CharacterLiterals,UserDefinedLiterals and NullPtrs
Simplify the Options names from "AddCommentsToXXX" to "CommentXXX"
Add extra unit tests to support additional supported literal types

MyDeveloperDay marked 6 inline comments as done.Feb 4 2019, 1:36 PM
aaron.ballman added inline comments.Feb 5 2019, 11:17 AM
clang-tidy/bugprone/ArgumentCommentCheck.cpp
228–236 ↗(On Diff #185148)

What's going on with these? Why not return isa<Blah>(Arg->IgnoreImpCasts()); (at which point, no need for the separate functions).

312–318 ↗(On Diff #185148)

This is a bit... much. I'm not certain what would be a better approach, but at the very least, factoring this out into a simple function call should make it a bit less... much. :-)

clang-tidy/bugprone/ArgumentCommentCheck.h
43–50 ↗(On Diff #185148)

At this point, I think it would be better to turn these into: const unsigned Blah : 1;

docs/clang-tidy/checks/bugprone-argument-comment.rst
40 ↗(On Diff #185014)

This still seems to be happening in the current revision?

MyDeveloperDay marked 6 inline comments as done.

Address review comments

clang-tidy/bugprone/ArgumentCommentCheck.cpp
228–236 ↗(On Diff #185148)

OK, my bad, I was just looking at the ast-dump on godbolt.org thinking... how do I get past that ImplicitCasrExpr, learning these tricks in the AST isn't always obvious despite me looking in doxygen, when you don't know what to look for its hard to know..but this is a neat trick and I'm happy to learn.

docs/clang-tidy/checks/bugprone-argument-comment.rst
40 ↗(On Diff #185014)

Sorry didn't catch your meaning but I assume it was the space between the arguments...

MyDeveloperDay marked an inline comment as done.Feb 5 2019, 1:15 PM
MyDeveloperDay added inline comments.
clang-tidy/bugprone/ArgumentCommentCheck.cpp
257 ↗(On Diff #185385)

@aaron.ballman, slight aside (and not in the code I introduced), when I run clang-tidy on the code I'm sending for review, I get the following...

C:\clang\llvm\tools\clang\tools\extra\clang-tidy\bugprone\ArgumentCommentCheck.cpp:253:8: warning: invalid case style for variable 'makeFileCharRange' [readability-identifier-naming]
  auto makeFileCharRange = [Ctx](SourceLocation Begin, SourceLocation End) {
       ^~~~~~~~~~~~~~~~~
       MakeFileCharRange

according to the .clang-tidy, a variable should be CamelCase, and a function camelBack, as its a Lambda what do we consider it should be? and does this really require an improvement in readability-identifier-naming? (might be something I'd be happy to look at next)

Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,readability-identifier-naming'
CheckOptions:
  - key:             readability-identifier-naming.ClassCase
    value:           CamelCase
  - key:             readability-identifier-naming.EnumCase
    value:           CamelCase
  - key:             readability-identifier-naming.FunctionCase
    value:           camelBack
  - key:             readability-identifier-naming.MemberCase
    value:           CamelCase
  - key:             readability-identifier-naming.ParameterCase
    value:           CamelCase
  - key:             readability-identifier-naming.UnionCase
    value:           CamelCase
  - key:             readability-identifier-naming.VariableCase
    value:           CamelCase
aaron.ballman added inline comments.Feb 6 2019, 6:07 AM
clang-tidy/bugprone/ArgumentCommentCheck.cpp
228–236 ↗(On Diff #185148)

No worries, I was just wondering if there was something special about a FullExpr that you didn't want to look though it for some reason. Glad to show you a new trick!

1 ↗(On Diff #185385)

Why did this get removed?

231 ↗(On Diff #185385)

I think we may want to do some additional work here. Consider:

#define FOO 1

void g(int);
void h(double);
void i(const char *);

void f() {
  g(FOO); // Probably don't want to suggest comments here
  g((1)); // Probably do want to suggest comments here
  h(1.0f); // Probably do want to suggest comments here
  i(__FILE__); // Probably don't want to suggest comments here
}

I think this code should do two things: 1) check whether the location of the arg is a macro expansion (if so, return false), 2) do Arg = Arg->IgnoreParenImpCasts(); at the start of the function and drop the Arg->IgnoreImpCasts() for the string and pointer literal cases. However, that may be a bridge too far, so you should add a test case like this:

g(_Generic(0, int : 0)); // Definitely do not want to see comments here

If it turns out the above case gives the comment suggestions, then I'd go with IgnoreImpCasts() rather than IgnoreParenImpCasts().

257 ↗(On Diff #185385)

according to the .clang-tidy, a variable should be CamelCase, and a function camelBack, as its a Lambda what do we consider it should be? and does this really require an improvement in readability-identifier-naming? (might be something I'd be happy to look at next)

Lambdas are variables, so I would expect those to follow the variable naming conventions. This is consistent with how we treat variables of function pointer type as well.

clang-tidy/bugprone/ArgumentCommentCheck.h
43 ↗(On Diff #185385)

Can you include this in the bit-field packing as well (with type unsigned)?

docs/clang-tidy/checks/bugprone-argument-comment.rst
40 ↗(On Diff #185014)

Yup, it was!

MyDeveloperDay marked 5 inline comments as done.

-Address review comments
-Ignore argument comment addition for macro inputs
-Add unit tests to cover the above
-Fix clang-tidy suggestion (from readability-identifier-naming)
-Ensure file is clang-formatted

MyDeveloperDay marked 3 inline comments as done.Feb 6 2019, 1:40 PM
MyDeveloperDay added inline comments.
clang-tidy/bugprone/ArgumentCommentCheck.cpp
1 ↗(On Diff #185385)

Sorry I suspect poor vim skills

231 ↗(On Diff #185385)

I agree, tried all combinations but g((1); and g(_Generic(0, int : 0)); would otherwise get comments

aaron.ballman accepted this revision.Feb 7 2019, 1:41 PM

LGTM aside from a testing nit.

clang-tidy/bugprone/ArgumentCommentCheck.cpp
231 ↗(On Diff #185385)

Yeah, I kind of figured that would be the case. Thank you for checking!

test/clang-tidy/bugprone-argument-comment-literals.cpp
104 ↗(On Diff #185632)

Can you add a FIXME comment that says we'd like to handle this case at some point? Maybe move the test closer to the _Generic example so you can mention that case as being a problem when handling paren expressions.

This revision is now accepted and ready to land.Feb 7 2019, 1:41 PM
MyDeveloperDay marked 2 inline comments as done.

Pre-Commit Changes

  • add FIXME wording to test
  • move local header include after libraries
  • rebase
MyDeveloperDay marked an inline comment as done.Feb 8 2019, 4:51 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 9:00 AM