Page MenuHomePhabricator

[clang-tidy] Add check readability-redundant-declaration
ClosedPublic

Authored by danielmarjamaki on Sep 16 2016, 4:11 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

danielmarjamaki retitled this revision from to [clang-tidy] Add check readability-redundant-declaration.
danielmarjamaki updated this object.
danielmarjamaki added a reviewer: alexfh.
danielmarjamaki added a subscriber: cfe-commits.

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.

danielmarjamaki marked 2 inline comments as done.Sep 16 2016, 4:26 AM
Eugene.Zelenko set the repository for this revision to rL LLVM.Sep 16 2016, 10:22 AM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

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?

danielmarjamaki removed rL LLVM as the repository for this revision.

run clang-format on test. add release notes.

danielmarjamaki marked an inline comment as done.Sep 19 2016, 12:48 AM
alexfh requested changes to this revision.Sep 23 2016, 7:22 PM
alexfh edited edge metadata.

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);
This revision now requires changes to proceed.Sep 23 2016, 7:22 PM
danielmarjamaki edited edge metadata.

Fix review comments

danielmarjamaki marked 3 inline comments as done.Sep 28 2016, 5:03 AM
danielmarjamaki added inline comments.
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.

danielmarjamaki marked 2 inline comments as done.Oct 5 2016, 12:51 AM

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.

alexfh requested changes to this revision.Oct 10 2016, 12:13 PM
alexfh edited edge metadata.
alexfh added inline comments.
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.

This revision now requires changes to proceed.Oct 10 2016, 12:13 PM
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();
               ^
aaron.ballman added inline comments.Oct 12 2016, 5:10 AM
clang-tidy/readability/RedundantDeclarationCheck.cpp
60 ↗(On Diff #72802)

diag(...) << cast<NamedDecl>(D) is required; the diagnostic engine properly handles quoting NamedDecl objects.

danielmarjamaki edited edge metadata.
danielmarjamaki set the repository for this revision to rL LLVM.

changed cast<NamedDecl>(D)->getName() to cast<NamedDecl>(D)

alexfh accepted this revision.Oct 22 2016, 1:08 PM
alexfh edited edge metadata.

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:

  • kinds of declarations supported by the check (function declarations aren't mentioned);
  • how declarations in different files are treated;
  • limitations (e.g. declarations with multiple variables);
  • a few words on the motivation for this check.
This revision is now accepted and ready to land.Oct 22 2016, 1:08 PM
This revision was automatically updated to reflect the committed changes.
alexfh added inline comments.Nov 2 2016, 12:45 AM
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).