alexfh added a comment.
Thank you for the fixes. I really like this way of structuring the warnings
more. We might need to polish the text in the messages a bit, and maybe also
change the case without definitions, but overall this seems fine. I have a few
more comments.
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:106
@@ +105,3 @@
+ if (IsTemplateSpecialization) {
+ // Template specializations need special handling.
+ // What we actually see here is a generated declaration from the main
----------------
piotrdz wrote:
> alexfh wrote:
> > I don't see why we couldn't just ignore compiler-generated template
> > specializations (hand-written ones should probably be handled). It should
> > be easy to filter them out by adding `unless(isInTemplateInstantiation())`
> > inside the `functionDecl` matcher.
> Unfortunately, it doesn't work like that. We get this declaration not from
> matcher, but by iterating redecls().
>
> In any case, I reconsidered this problem, and I came to conclusion that since
> we need special code for handling template specializations, we may as well do
> it properly. The only reason that this version of code works at all, is
> because of incidental generation of this special declaration appearing in the
> same place where we have function specialization. This, I think, should be
> considered a side effect of how AST generation works now, and not how it must
> necessarily work. It may change or even disappear in the future, making this
> code brittle.
>
> To do this correctly, I believe we should retrieve main template declaration
> by using `getPrimaryTemplate()->getTemplatedDecl()` and process that. This
> fixes the issue of wrong location reporting, and also makes it clear what is
> happening to someone reading the code.
>
> So in the end, partly because of this issue, I ended up rewriting over half
> of my code, but what we're left with is I think much better.
I see now. Thanks for explaining!
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:271
@@ +270,3 @@
+void InconsistentDeclarationParameterNameCheck::
+ formatDiagnosticsInCaseOfOnlyFunctionDeclarationsPresent(
+ const FormatDiagnosticContext &Context) {
----------------
The name is too long for my taste. It doesn't look well with 80-columns limit.
Maybe `formatDiagnosticsForDeclarations` or something like this?
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:275
@@ +274,3 @@
+ diag(Context.OriginalDeclaration->getLocation(),
+ "function %q0 has %1 other declaration%s1 with differently named "
+ "parameters")
----------------
I'm not a native speaker, but "with differently named parameters" doesn't sound
well to me. Maybe "with different parameter names"?
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:289
@@ +288,3 @@
+
+ formatDifferingParamsDiagnostic(FormatParamsDiagnosticContext{
+ InconsistentDeclaration.DeclarationLocation, "other declaration",
----------------
I really don't like this idea of introducing a structure for the sole purpose
of passing three parameters to a function. I don't see what we gain by doing
so, and I would prefer the usual way to pass parameters. The same argument is
valid for `formatDiagnosticsInCaseOfOnlyFunctionDeclarationsPresent` and
`formatDiagnosticsInOtherCases`.
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:290
@@ +289,3 @@
+ formatDifferingParamsDiagnostic(FormatParamsDiagnosticContext{
+ InconsistentDeclaration.DeclarationLocation, "other declaration",
+ InconsistentDeclaration.DifferingParams});
----------------
Maybe "the other declaration"?
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:297
@@ +296,3 @@
+
+void InconsistentDeclarationParameterNameCheck::formatDiagnosticsInOtherCases(
+ const FormatDiagnosticContext &Context, StringRef FunctionDescription,
----------------
The name `formatDiagnosticsInOtherCases` only makes sense when you have the
other function name in mind
(`formatDiagnosticsInCaseOfOnlyFunctionDeclarationsPresent`). I don't have a
good name for it, but even just removing `InOtherCases` would make things
better.
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:307
@@ +306,3 @@
+
+ diag(Context.ParameterSourceDeclaration->getLocation(), "%0 seen here",
+ DiagnosticIDs::Level::Note)
----------------
Maybe "the %0 seen here"?
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:326
@@ +325,3 @@
+ diag(Context.Location,
+ "differing parameters are named here: (%0), while in %1: (%2)",
+ DiagnosticIDs::Level::Note)
----------------
I'd remove "while".
http://reviews.llvm.org/D12462
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
