[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Did you decide on fuchsia instead of zircon for the module name, or is that decision still up in the air?



================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:22
+  // Calling a function which uses default arguments is disallowed.
+  Finder->addMatcher(cxxDefaultArgExpr().bind("stmt"), this);
+  // Declaring default parameters is disallowed.
----------------
juliehockett wrote:
> aaron.ballman wrote:
> > Isn't this case already covered by the other matcher? Or do you have system header files which have default arguments and those functions are disallowed by the style guide, but cannot be removed from the system header?
> The latter -- there's a bunch of third-party code and the like that could have default arguments, but shouldn't be used like that in the project.
Ah, that makes sense. Thanks!


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:29
+    diag(S->getUsedLocation(),
+         "calling functions which use default arguments are disallowed");
+    diag(S->getParam()->getLocStart(), 
----------------
I think the diagnostic should be in singular form. How about: `calling a function that uses a default argument is disallowed`?


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:35-38
+    SourceRange RemovalRange(
+          Lexer::getLocForEndOfToken(D->getLocation(), 0, 
+                *Result.SourceManager, Result.Context->getLangOpts()),
+          D->getDefaultArgRange().getEnd()
----------------
Does `getDefaultArgRange()` not provide the correct range already (so you don't need to go back to the original source)? Or does that range miss the `=`?

You might want to disable the fix-it in the case `getDefaultArgRange()` returns an empty range, or in case the default arg expression comes from a macro (and add a test for the latter case). e.g.,
```
#define DERP(val)  = val

void f(int i DERP);
```
Additionally, a test where the identifier is elided would be good as well: `void f(int = 12);`


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:41
+    diag(D->getLocStart(),
+         "declaring functions which use default arguments are disallowed")
+         << D
----------------
This diagnostic could be similarly tightened -- the function isn't the issue, the parameter with the default argument is. Perhaps: `declaring a parameter with a default value is disallowed`?


================
Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:16-17
+
+If a function with default arguments is already defined, calling it with no
+arguments will also cause a warning. Calling it without defaults will not cause
+a warning:
----------------
The wording here is a bit confusing; it's not that calling it with no arguments causes the warning, it's that calling it such that the default argument value is used that's the problem. Perhaps: `A function call expression that uses a default argument will be diagnosed.` or something along those lines.


https://reviews.llvm.org/D40108