This is a new check that warns about redundant variable declarations.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
For information, I am testing this on debian packages right now. I will see the results next week.
clang-tidy/readability/RedundantDeclarationCheck.cpp | ||
---|---|---|
22 ↗ | (On Diff #71612) | I forgot to remove this FIXME comment, I will remove it. |
41 ↗ | (On Diff #71612) | Imho this is a clumpsy way to see if it's a "multivariable" declaration. Do you know if there is a better way? |
50 ↗ | (On Diff #71612) | Is there a better way to rewrite this ... I tried something like this without success: auto D = diag(..); if (!MultiVar) D << FixItHint..; |
clang-tidy/readability/RedundantDeclarationCheck.h | ||
19 ↗ | (On Diff #71612) | I will fix this FIXME. |
test/clang-tidy/readability-redundant-declaration.cpp | ||
11 ↗ | (On Diff #71612) | Ideally this would be changed to "extern int B;". I currently don't fix multivariable declarations. |
Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).
Will be good idea to detect redundant function prototypes.
However, I think this check should be part of Clang diagnostics. GCC has -Wredundant-decls for a long time.
test/clang-tidy/readability-redundant-declaration.cpp | ||
---|---|---|
9 ↗ | (On Diff #71614) | Please run Clang-format over test. |
However, I think this check should be part of Clang diagnostics. GCC has -Wredundant-decls for a long time.
I am not against that.
What is the criteria? When should it be in the compiler and when should it be in clang-tidy?
Personally I think it's natural that the compiler has warnings about dangerous code. This check is about readability.
Will be good idea to detect redundant function prototypes.
Yes.
Should that have the same ID though? Is it better to have one readability-redundant-declaration or two separate readability-redundant-variable-declaration and readability-redundant-function-declaration?
If this kind of an error is not too frequent, it might make sense as a clang diagnostic, indeed. Having it in clang-tidy until then doesn't hurt, though.
clang-tidy/readability/RedundantDeclarationCheck.cpp | ||
---|---|---|
36 ↗ | (On Diff #71775) | Don't check for system headers, clang-tidy takes care of this (and sometimes, warnings in system headers are interesting to the standard library maintainers). |
38 ↗ | (On Diff #71775) | Don't check for the same file. It will prevent fixing headers. |
44 ↗ | (On Diff #71775) | There should be a better way of checking for multi variable declarations. |
58–64 ↗ | (On Diff #71775) | No need to duplicate this code. Store the diagnostic builder and conditionally insert the fix: { auto Diag = diag(...); if (x) Diag << FixItHint::....; } diag(..., Note); |
clang-tidy/readability/RedundantDeclarationCheck.cpp | ||
---|---|---|
39 ↗ | (On Diff #72802) | I don't want to generate a fixit. because it could easily break the code. And it will probably depend on inclusion order. |
45 ↗ | (On Diff #72802) | I think this is better. But not perfect. Maybe there is still a better way. |
59–65 ↗ | (On Diff #72802) | Thanks! That works. |
Ping.
I am not very happy about how I detect multivariable declarations. But I really don't see any such info in the VarDecl. For instance, the AST dump does not show this info.
clang-tidy/readability/RedundantDeclarationCheck.cpp | ||
---|---|---|
39 ↗ | (On Diff #72802) | Yep, not generating fixes for redundant declarations in multiple headers totally makes sense (at least, by default). |
45 ↗ | (On Diff #72802) | I don't know whether there is a better way. I somewhat dislike iterating over all declarations here, but it only happens when we're about to generate a warning anyway. Good enough for the start. |
60 ↗ | (On Diff #72802) | It should be possible to just use D here. |
clang-tidy/readability/RedundantDeclarationCheck.cpp | ||
---|---|---|
60 ↗ | (On Diff #72802) | Thanks. It's not possible: RedundantDeclarationCheck.cpp:61:15: error: ‘const class clang::Decl’ has no member named ‘getName’ << D->getName(); ^ |
clang-tidy/readability/RedundantDeclarationCheck.cpp | ||
---|---|---|
60 ↗ | (On Diff #72802) | diag(...) << cast<NamedDecl>(D) is required; the diagnostic engine properly handles quoting NamedDecl objects. |
Sorry for the delay. I'm on vacation, returning on Monday.
The patch looks good when the comments are fixed.
clang-tidy/readability/RedundantDeclarationCheck.cpp | ||
---|---|---|
36 ↗ | (On Diff #74478) | nit: No need for parentheses here. |
41 ↗ | (On Diff #74478) | VD is checked on the previous line, no need to repeat the check here. |
61 ↗ | (On Diff #74478) | Looks like it's better to make D NamedDecl and remove the cast here: Finder->addMatcher(namedDecl(... ... const auto *D = Result.Nodes.getNodeAs<NamedDecl>("Decl"); |
docs/clang-tidy/checks/readability-redundant-declaration.rst | ||
6 ↗ | (On Diff #74478) | I think, these points should be covered more thoroughly:
|
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp | ||
---|---|---|
11 | Just noticed: could you add a check line for the note ("previously declared here")? One should be enough. | |
17 | Just noticed: this is a rather unspecific check pattern that can match any empty line anywhere in the file. Int way to make the check stricter is to add a unique trailing comment and then match it (anchored to the line start). |
Just noticed: could you add a check line for the note ("previously declared here")? One should be enough.