> Try running only your check (clang-tidy -checks=<your check name>). This
way you don't get rid of compiler warnings (yet), but they shouldn't appear in
clang-tidy if your build is clean.
Sorry, I meant that I received benign warnings from my checker as a result of
analyzing code in the AST library. Anyway, I fixed those locally and spotted a
few more interesting warnings.
/home/peter/src/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:537:19:
warning: argument name 'isPrologue' in comment does not match parameter name
'isStore' [llvm-argument-comment]
emitFrameMemOps(/* isPrologue = */ true, MBB, MBBI, CSI, TRI,
^
/home/peter/src/llvm/lib/Target/AArch64/AArch64FrameLowering.h:89:29: note:
'isStore' declared here [llvm-argument-comment]
void emitFrameMemOps(bool isStore, MachineBasicBlock &MBB,
^
/home/peter/src/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:558:19:
warning: argument name 'isPrologue' in comment does not match parameter name
'isStore' [llvm-argument-comment]
emitFrameMemOps(/* isPrologue = */ false, MBB, MBBI, CSI, TRI,
^
/home/peter/src/llvm/lib/Target/AArch64/AArch64FrameLowering.h:89:29: note:
'isStore' declared here [llvm-argument-comment]
void emitFrameMemOps(bool isStore, MachineBasicBlock &MBB,
^
/home/peter/src/llvm/tools/clang/lib/Sema/SemaExpr.cpp:1099:37: warning:
argument name 'convertInt' in comment does not match parameter name
'ConvertFloat' [llvm-argument-comment]
/*convertInt=*/ true,
^
/home/peter/src/llvm/tools/clang/lib/Sema/SemaExpr.cpp:1044:49: note:
'ConvertFloat' declared here [llvm-argument-comment]
bool ConvertFloat, bool
ConvertInt) {
^
/home/peter/src/llvm/tools/clang/lib/Sema/SemaExpr.cpp:1100:37: warning:
argument name 'convertFloat' in comment does not match parameter name
'ConvertInt' [llvm-argument-comment]
/*convertFloat=*/!IsCompAssign);
^
/home/peter/src/llvm/tools/clang/lib/Sema/SemaExpr.cpp:1044:68: note:
'ConvertInt' declared here [llvm-argument-comment]
bool ConvertFloat, bool
ConvertInt) {
^
> Moving checkers around doesn't seem a very complicated refactoring, so
there's no harm in waiting until we have more checkers.
Sounds reasonable.
> FYI, in r202970 I've added a directory for various checks not related to a
particular coding style. You can put the check there until we decide whether it
belongs to LLVM style or not.
Thanks for taking care of that. I've moved my check there.
================
Comment at: clang-tidy/llvm/LLVMTidyModule.cpp:140
@@ +139,3 @@
+ const Expr *E = Result.Nodes.getNodeAs<Expr>("expr");
+ if (auto CE = dyn_cast<CallExpr>(E)) {
+ auto FD = CE->getDirectCallee();
----------------
Alexander Kornienko wrote:
> Have we moved to the Almost Always Auto style? ;)
>
> I'd say that the usages of auto in this method don't add much value (unlike
> the one on lines 121 and 123). Could you use concrete types here instead?
>
> I would also expand CE, FD and CCE to a bit more meaningful names.
Renamed.
I think the style lets us use auto where it is obvious from the context what
the type is. On lines 140 and 149 the type is named in the cast expression so I
think we can use auto there but I guess it isn't completely obvious for line
141 so I changed it.
================
Comment at: test/clang-tidy/llvm-arg-comments.cpp:1
@@ +1,2 @@
+// RUN: clang-tidy --checks=llvm-argument-comment %s -- > %t.cpp
+// RUN: FileCheck -input-file=%t.cpp %s
----------------
Alexander Kornienko wrote:
> You could also avoid using a temporary file and just pipe the clang-tidy
> output to FileCheck.
Done.
================
Comment at: test/clang-tidy/llvm-arg-comments.cpp:3
@@ +2,3 @@
+// RUN: FileCheck -input-file=%t.cpp %s
+
+void f(int x, int y);
----------------
Alexander Kornienko wrote:
> Please add
>
> // CHECK-NOT: warning
>
> here and in the end of the file. I didn't find a better way to check for the
> absence of the unexpected diagnostics. Maybe we should implement -verify in
> clang-tidy as it's done in Clang.
Done.
================
Comment at: test/clang-tidy/llvm-arg-comments.cpp:7
@@ +6,3 @@
+void g() {
+ // CHECK: 11:5: warning: argument name 'y' in comment does not match
parameter name 'x'
+ // CHECK: 4:12: note: 'x' declared here
----------------
Alexander Kornienko wrote:
> You can use [[@LINE+4]] instead of "11" here to make the test easier to
> change.
Done.
================
Comment at: clang-tidy/llvm/LLVMTidyModule.cpp:80
@@ +79,3 @@
+
+ while (1) {
+ Token Tok;
----------------
Alexander Kornienko wrote:
> Maybe while (true)? Or even:
>
> Token Tok;
> while (!TheLexer.LexFromRawLexer(Tok) && Tok.getLocation() !=
> Range.getEnd() && Tok.getKind() != tok::eof()) {
> ...
>
> ?
I would prefer to keep the checks in the loop body as I consider it to be more
readable.
================
Comment at: clang-tidy/llvm/LLVMTidyModule.h:48
@@ +47,3 @@
+public:
+ void registerMatchers(ast_matchers::MatchFinder *Finder) LLVM_OVERRIDE;
+ void
----------------
Alexander Kornienko wrote:
> There's no LLVM_OVERRIDE already. Use "override". Things are going fast these
> days. ;)
Done.
================
Comment at: clang-tidy/llvm/LLVMTidyModule.cpp:131
@@ +130,3 @@
+ diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note)
+ << II;
+ }
----------------
Alexander Kornienko wrote:
> I think, adding a FixIt to correct the comment would be useful at least in
> some of the cases (say, a typo in the comment). As a safety measure you could
> try to find the argument with the closest name to the one used in the
> comment, and only suggest the fix if this is the same argument. What do you
> think?
That sounds about right but I think we should put an upper bound on the
allowable edit distance (similarly to how we handle typos during semantic
analysis) and use a larger threshold for the other parameters. I've implemented
this scheme and I'll let it run over LLVM tonight.
================
Comment at: clang-tidy/llvm/LLVMTidyModule.h:39
@@ +38,3 @@
+class ArgumentCommentCheck : public ClangTidyCheck {
+ llvm::Regex IdentRE{ "^/\\* *([_A-Za-z][_A-Za-z0-9]*) *= *\\*/$" };
+
----------------
Alexander Kornienko wrote:
> I'd move the private section to the end of the class.
Done.
http://llvm-reviews.chandlerc.com/D2914
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
