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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
