readability-redundant-declaration: fix false positive for static member declaration

[danielmarjamaki]

Summary

Fix a false positive in the readability-redundant-declaration.

Example code:

struct S {
  static int I;
}:
int S::I;  // <- don't warn

For information, I also ran clang-format on the files and got some extra "unintentional" style updates.

[hokein]

Thanks for the fix.

clang-tidy/readability/RedundantDeclarationCheck.cpp

48 ↗

(On Diff #79078)

Any reason to use lexical DC here? I think we should use Prev->getDeclContext() to get the semantic DC.

[danielmarjamaki]

clang-tidy/readability/RedundantDeclarationCheck.cpp

48 ↗

(On Diff #79078)

Thanks! No there was no reason, it was a mistake.

[danielmarjamaki]

Fix review comments

[hokein]

clang-tidy/readability/RedundantDeclarationCheck.cpp

22 ↗

(On Diff #79297)

Some thoughts about the ast matcher:

Since you only want to match function/variable declarations (not definitions) in this check, we could add unless(isDefintion()) in the matcher which can make the check more efficient.

And for varDecl, I think you should also ignore static variables?

49 ↗

(On Diff #79297)

Why do you use getRedeclContext? Doesn't DC->isRecord() work?

[danielmarjamaki]

clang-tidy/readability/RedundantDeclarationCheck.cpp

22 ↗

(On Diff #79297)

Sounds reasonable. Probably I did it wrongly.. I get this now:

.../ASTMatchers.h:3919:44: error: ‘const class clang::NamedDecl’ has no member named ‘isThisDeclarationADefinition’
   return Node.isThisDeclarationADefinition();

I will try to make it work.

Maybe I misunderstood.. I want to warn about redundant static variable declarations in module scope.

static int X;
static int X;

49 ↗

(On Diff #79297)

It is just because I don't know all the possible shortcuts. I will try DC->isRecord() but I assume it works too.

[hokein]

clang-tidy/readability/RedundantDeclarationCheck.cpp

22 ↗

(On Diff #79297)

.../ASTMatchers.h:3919:44: error: ‘const class clang::NamedDecl’ has no member named ‘isThisDeclarationADefinition’
   return Node.isThisDeclarationADefinition();

I see you error. isDefinition() can't apply to nameDecl directly. So you need replicate the machter like namedDecl(anyOf(varDecl(unless(isDefinition())), functionDecl(unless(isDefinition()))) (But you can still simplify it by making unless(isDefinition()) a variable).

Maybe I misunderstood.. I want to warn about redundant static variable declarations in module scope.

hmm... I believe there is a redefinition error in your given case, if you declare "static int X;" twice.

[danielmarjamaki]

clang-tidy/readability/RedundantDeclarationCheck.cpp

22 ↗

(On Diff #79297)

hmm... I believe there is a redefinition error in your given case, if you declare "static int X;" twice.

well.. gcc/clang does not complain about it. hmm.. I don't see a clang-tidy warning about redundant declaration neither.

[hokein]

clang-tidy/readability/RedundantDeclarationCheck.cpp

22 ↗

(On Diff #79297)

Here is my results:

$  cat test.cpp
static int i;
static int i;
$ clang test.cpp
test.cpp:2:12: error: redefinition of 'i'
static int i;
           ^
test.cpp:1:12: note: previous definition is here
static int i;
           ^
1 error generated.

[danielmarjamaki]

clang-tidy/readability/RedundantDeclarationCheck.cpp

22 ↗

(On Diff #79297)

Yes. Well there is C also.

$  cat test.cpp
static int i;
static int i;
$ clang -fsyntax-only test.cpp
test.cpp:2:12: error: redefinition of 'i'
static int i;
           ^
test.cpp:1:12: note: previous definition is here
static int i;
           ^
1 error generated.
$ cp test.cpp test.c
$ clang -fsyntax-only test.c
$

[danielmarjamaki]

Fixed review comments

[alexfh]

test/clang-tidy/readability-redundant-declaration.cpp

22 ↗

(On Diff #79297)

This CHECK line will match any empty line, which is not what you want. Add a comment after the declaration and match it anchored to the start of line. Same elsewhere.

[danielmarjamaki]

Fix review comment about CHECK-FIXES regexp pattern.

[danielmarjamaki]

Ping

[hokein]

Sorry for the delay. LGTM, thanks.

Nit: Please mark the reviewers' comments Done in Phabricator once you address them, it will make review stuff easier.

clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 void RedundantDeclarationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(namedDecl(anyOf(varDecl(), functionDecl())).bind("Decl"),
+  auto UnlessDefinition = unless(isDefinition());
+  Finder->addMatcher(namedDecl(anyOf(varDecl(UnlessDefinition),
+                                     functionDecl(UnlessDefinition)))
+                         .bind("Decl"),
                      this);
 }
 
 void RedundantDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *D = Result.Nodes.getNodeAs<NamedDecl>("Decl");
   const auto *Prev = D->getPreviousDecl();
   if (!Prev)
     return;
   if (!Prev->getLocation().isValid())
     return;
   if (Prev->getLocation() == D->getLocation())
     return;
 
   const SourceManager &SM = *Result.SourceManager;
 
   const bool DifferentHeaders =
       !SM.isInMainFile(D->getLocation()) &&
       !SM.isWrittenInSameFile(Prev->getLocation(), D->getLocation());
 
   bool MultiVar = false;
   if (const auto *VD = dyn_cast<VarDecl>(D)) {
-    if (VD->getPreviousDecl()->getStorageClass() == SC_Extern &&
-        VD->getStorageClass() != SC_Extern)
-      return;
     // Is this a multivariable declaration?
     for (const auto Other : VD->getDeclContext()->decls()) {
       if (Other != D && Other->getLocStart() == VD->getLocStart()) {
         MultiVar = true;
         break;
       }
     }
-  } else {
-    const auto *FD = cast<FunctionDecl>(D);
-    if (FD->isThisDeclarationADefinition())
-      return;
   }
 
   SourceLocation EndLoc = Lexer::getLocForEndOfToken(
       D->getSourceRange().getEnd(), 0, SM, Result.Context->getLangOpts());
   {
     auto Diag = diag(D->getLocation(), "redundant %0 declaration") << D;
     if (!MultiVar && !DifferentHeaders)
       Diag << FixItHint::CreateRemoval(
           SourceRange(D->getSourceRange().getBegin(), EndLoc));
   }
   diag(Prev->getLocation(), "previously declared here", DiagnosticIDs::Note);
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp

 // RUN: %check_clang_tidy %s readability-redundant-declaration %t
 
 extern int Xyz;
-extern int Xyz;
+extern int Xyz; // Xyz
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'Xyz' declaration [readability-redundant-declaration]
-// CHECK-FIXES: {{^}}{{$}}
+// CHECK-FIXES: {{^}}// Xyz{{$}}
 int Xyz = 123;
 
 extern int A;
 extern int A, B;
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'A' declaration
 // CHECK-FIXES: {{^}}extern int A, B;{{$}}
 
 extern int Buf[10];
-extern int Buf[10];
+extern int Buf[10]; // Buf[10]
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'Buf' declaration
-// CHECK-FIXES: {{^}}{{$}}
+// CHECK-FIXES: {{^}}// Buf[10]{{$}}
 
 static int f();
-static int f();
+static int f(); // f
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'f' declaration
-// CHECK-FIXES: {{^}}{{$}}
+// CHECK-FIXES: {{^}}// f{{$}}
 static int f() {}
 
 // Original check crashed for the code below.
 namespace std {
-  typedef decltype(sizeof(0)) size_t;
 }
-void* operator new(std::size_t) __attribute__((__externally_visible__));
-void* operator new[](std::size_t) __attribute__((__externally_visible__));
+void *operator new(std::size_t) __attribute__((__externally_visible__));
+void *operator new[](std::size_t) __attribute__((__externally_visible__));
+
+// Don't warn about static member definition.
+struct C {
+  static int I;
+};
+int C::I;