[PATCH] Add an argument comment checker to clang-tidy.

[Peter Collingbourne]

  > 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