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

[Alexander Kornienko]

================
Comment at: clang-tidy/misc/MiscTidyModule.h:1
@@ +1,2 @@
+//===--- MiscTidyModule.h - clang-tidy --------------------------*- C++ -*-===//
+//
----------------
I'd make a .h + .cpp file per check instead of using a single header for all checks.

================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:122
@@ +121,3 @@
+
+    auto Comments =
+        getCommentsInRange(Ctx, SourceRange(BeginSLoc, EndSLoc));
----------------
Maybe "for (auto Comment : getCommentsInRange(...))"?

================
Comment at: test/clang-tidy/arg-comments.cpp:26
@@ +25,3 @@
+}
+
+// CHECK-NOT: warning
----------------
Please add tests for the typo detection logic.

It may be easier to write replacement tests as unit-tests, than as lit-based ones. See tools/extra/unittests/clang-tidy/GoogleModuleTest.cpp for an example.

================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:88
@@ +87,3 @@
+
+    // Other parameters must be an edit distance at least 2 more away from this
+    // parameter. This gives us greater confidence that this is a typo of this
----------------
nit: I'd make a constant for "2" in case it turns out, that 1 or 3 works better here.

================
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();
----------------
Peter Collingbourne wrote:
> 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.
Sounds reasonable.

================
Comment at: clang-tidy/llvm/LLVMTidyModule.cpp:131
@@ +130,3 @@
+          diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note)
+              << II;
+        }
----------------
Peter Collingbourne wrote:
> 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.
That's even better. BTW, if you run this over LLVM in fix mode, you'll probably be disappointed, as 1. there's a problem with replacement fix-its (I'll commit a patch soon), 2. fixing inside headers makes awful things without deduplication (I'll try to work this out today). As for the diagnostic run, I found it to be quite fast (faster, than a full build), when running in parallel with xargs -P, e.g.:

  find lib/ -name "*.cpp" | xargs -n1 -P32 clang-tidy -checks=misc-

(if you use this already, sorry for the noise).


http://llvm-reviews.chandlerc.com/D2914