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

[danielmarjamaki]

Summary

This is a new check that warns about redundant variable declarations.

[danielmarjamaki]

For information, I am testing this on debian packages right now. I will see the results next week.

clang-tidy/readability/RedundantDeclarationCheck.cpp

22

I forgot to remove this FIXME comment, I will remove it.

41

Imho this is a clumpsy way to see if it's a "multivariable" declaration. Do you know if there is a better way?

50

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

I will fix this FIXME.

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

11

Ideally this would be changed to "extern int B;". I currently don't fix multivariable declarations.

[danielmarjamaki]

minor fixes

[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.

[Eugene.Zelenko]

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

10

Please run Clang-format over test.

[danielmarjamaki]

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.

[danielmarjamaki]

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]

run clang-format on test. add release notes.

[alexfh]

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

37

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).

39

Don't check for the same file. It will prevent fixing headers.

45

There should be a better way of checking for multi variable declarations.

59–65

No need to duplicate this code. Store the diagnostic builder and conditionally insert the fix:

{
  auto Diag = diag(...);
  if (x)
    Diag << FixItHint::....;
}
diag(..., Note);

[danielmarjamaki]

Fix review comments

[danielmarjamaki]

clang-tidy/readability/RedundantDeclarationCheck.cpp

40

I don't want to generate a fixit. because it could easily break the code. And it will probably depend on inclusion order.

46

I think this is better. But not perfect. Maybe there is still a better way.

60–66

Thanks! That works.

[danielmarjamaki]

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]

clang-tidy/readability/RedundantDeclarationCheck.cpp

40

Yep, not generating fixes for redundant declarations in multiple headers totally makes sense (at least, by default).

46

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.

61

It should be possible to just use D here.

[danielmarjamaki]

clang-tidy/readability/RedundantDeclarationCheck.cpp

61

Thanks. It's not possible:

RedundantDeclarationCheck.cpp:61:15: error: ‘const class clang::Decl’ has no member named ‘getName’
         << D->getName();
               ^

[aaron.ballman]

clang-tidy/readability/RedundantDeclarationCheck.cpp

61

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

[danielmarjamaki]

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

[danielmarjamaki]

Ping.

[alexfh]

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

37

nit: No need for parentheses here.

42

VD is checked on the previous line, no need to repeat the check here.

62

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

7

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.

[alexfh]

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

11 ↗

(On Diff #76548)

Just noticed: could you add a check line for the note ("previously declared here")? One should be enough.

17 ↗

(On Diff #76548)

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).

clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
+  RedundantDeclarationCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
+#include "RedundantDeclarationCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolCastCheck>(
         "readability-implicit-bool-cast");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
+    CheckFactories.registerCheck<RedundantDeclarationCheck>(
+        "readability-redundant-declaration");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<NonConstParameterCheck>(
         "readability-non-const-parameter");
     CheckFactories.registerCheck<RedundantControlFlowCheck>(
         "readability-redundant-control-flow");

clang-tidy/readability/RedundantDeclarationCheck.h

+//===--- RedundantDeclarationCheck.h - clang-tidy----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-declaration.html
+class RedundantDeclarationCheck : public ClangTidyCheck {
+public:
+  RedundantDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H

clang-tidy/readability/RedundantDeclarationCheck.cpp

+//===--- RedundantDeclarationCheck.cpp - clang-tidy------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantDeclarationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void RedundantDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: Add matchers.
+  Finder->addMatcher(varDecl().bind("Decl"), this);
+}
+
+void RedundantDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *VD = Result.Nodes.getNodeAs<VarDecl>("Decl");
+  const auto *Prev = VD->getPreviousDecl();
+  if (!Prev)
+    return;
+  if (Prev->getStorageClass() == SC_Extern && VD->getStorageClass() != SC_Extern)
+    return;
+  if (Prev->getLocation() == VD->getLocation())
+    return;
+  const SourceManager &SM = *Result.SourceManager;
+  if (SM.isInSystemHeader(VD->getLocation()))
+    return;
+  if (!SM.isInMainFile(VD->getLocation()) && !SM.isWrittenInSameFile(Prev->getLocation(), VD->getLocation()))
+    return;
+
+  // Don't generate fixits for multivariable declarations.
+  bool MultiVar = false;
+  for (const char *Code = SM.getCharacterData(VD->getLocStart()); *Code && *Code != ';'; ++Code) {
+    if (*Code == ',') {
+      MultiVar = true;
+      break;
+    }
+  }
+
+  if (MultiVar) {
+    diag(VD->getLocation(), "redundant variable %0 declaration")
+      << VD->getName();
+    diag(VD->getPreviousDecl()->getLocation(), "previously declared here", DiagnosticIDs::Note);
+  } else {
+    SourceLocation EndLoc = Lexer::getLocForEndOfToken(VD->getLocEnd(), 0, SM, Result.Context->getLangOpts());
+    diag(VD->getLocation(), "redundant variable %0 declaration")
+      << VD->getName()
+      << FixItHint::CreateRemoval(SourceRange(VD->getLocStart(), EndLoc));
+    diag(VD->getPreviousDecl()->getLocation(), "previously declared here", DiagnosticIDs::Note);
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-misplaced-array-index
    readability-named-parameter
    readability-non-const-parameter
    readability-redundant-control-flow
+   readability-redundant-declaration
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release

docs/clang-tidy/checks/readability-redundant-declaration.rst

+.. title:: clang-tidy - readability-redundant-declaration
+
+readability-redundant-declaration
+=================================
+
+Finds redundant variable declarations.
+
+.. code-block:: c++
+
+  extern int X;
+  extern int X;
+
+becomes
+
+.. code-block:: c++
+
+  extern int X;

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

+// RUN: %check_clang_tidy %s readability-redundant-declaration %t
+
+extern int Xyz;
+extern int Xyz;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable Xyz declaration [readability-redundant-declaration]
+// CHECK-FIXES: {{^}}{{$}}
+
+extern int A;
+extern int A,B;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable A declaration
+// CHECK-FIXES: {{^}}extern int A,B;{{$}}
+
+extern int Buf[10];
+extern int Buf[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable Buf declaration
+// CHECK-FIXES: {{^}}{{$}}