aaron.ballman added inline comments.
================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:228-236
+static bool isStringLiteral(const Expr *Arg) {
+ const auto *Cast = dyn_cast<ImplicitCastExpr>(Arg);
+ return Cast ? isa<StringLiteral>(Cast->getSubExpr()) : false;
+}
+
+static bool isNullPtrLiteral(const Expr *Arg) {
+ const auto *Cast = dyn_cast<ImplicitCastExpr>(Arg);
----------------
MyDeveloperDay wrote:
> aaron.ballman wrote:
> > What's going on with these? Why not `return
> > isa<Blah>(Arg->IgnoreImpCasts());` (at which point, no need for the
> > separate functions).
> 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.
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!
================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:1
-//===--- ArgumentCommentCheck.cpp - clang-tidy
----------------------------===//
//
----------------
Why did this get removed?
================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:231
+bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const {
+ return (CommentBoolLiterals && isa<CXXBoolLiteralExpr>(Arg)) ||
+ (CommentIntegerLiterals && isa<IntegerLiteral>(Arg)) ||
----------------
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()`.
================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:257
Ctx->getLangOpts());
};
----------------
MyDeveloperDay wrote:
> @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
> ```
>
>
>
>
>
>
>
>
> 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.
================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.h:43
private:
const bool StrictMode;
+ const unsigned CommentBoolLiterals : 1;
----------------
Can you include this in the bit-field packing as well (with type `unsigned`)?
================
Comment at: docs/clang-tidy/checks/bugprone-argument-comment.rst:40
+
+ void foo(bool turn_key,bool press_button);
+
----------------
MyDeveloperDay wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Format the code examples from our style guide as well (same below).
> > This still seems to be happening in the current revision?
> Sorry didn't catch your meaning but I assume it was the space between the
> arguments...
Yup, it was!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57674/new/
https://reviews.llvm.org/D57674
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
