Warn on explicit copy constructors.

[alexfh]

Summary

The Google C++ Style Guide doesn't require copy constructors to be
declared explicit, but some people do this by mistake. Make this check detect
and fix such cases.

[alexfh]

Added a separate variable for lambda, reformatted code.

[djasper]

clang-tidy/google/GoogleTidyModule.cpp

34

IMO, passing in a lambda is a slight overkill here, at least until this is actually used for a second purpose. But ok.

62

So, this is:

if (a || b) {
  if (a && b) {
    ..
  }
  return;
}

I think this is easier to follow as:

if (a && b) {
  ..
}
if (a || b)
  return;

And the latter if can actually be merged with the if concerning the parameter count.

clang-tidy/google/GoogleTidyModule.cpp

 #include "GoogleTidyModule.h"
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Lexer.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
 void ExplicitConstructorCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(constructorDecl().bind("ctor"), this);
 }
 
+// Looks for the token matching the predicate and returns the range of the found
+// token including trailing whitespace.
+SourceRange FindToken(const SourceManager &Sources, LangOptions LangOpts,
+                      SourceLocation StartLoc, SourceLocation EndLoc,
+                      bool (*Pred)(const Token &)) {
+  FileID File = Sources.getFileID(Sources.getSpellingLoc(StartLoc));
+  StringRef Buf = Sources.getBufferData(File);
+  const char *StartChar = Sources.getCharacterData(StartLoc);
+  Lexer Lex(StartLoc, LangOpts, StartChar, StartChar, Buf.end());
+  Lex.SetCommentRetentionState(true);
+  Token Tok;
+  do {
+    Lex.LexFromRawLexer(Tok);
+    if (Pred(Tok)) {
+      Token NextTok;
+      Lex.LexFromRawLexer(NextTok);
+      return SourceRange(Tok.getLocation(), NextTok.getLocation());
+    }
+  } while (Tok.isNot(tok::eof) && Tok.getLocation() < EndLoc);
+
+  return SourceRange();
+}
+
 void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) {
   const CXXConstructorDecl *Ctor =
       Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
   // Do not be confused: isExplicit means 'explicit' keyword is present,
   // isImplicit means that it's a compiler-generated constructor.
-  if (Ctor->isOutOfLine() || Ctor->isExplicit() || Ctor->isImplicit() ||
-      Ctor->isDeleted() || Ctor->isCopyOrMoveConstructor())
+  if (Ctor->isOutOfLine() || Ctor->isImplicit() || Ctor->isDeleted())
+    return;
+  if (Ctor->isExplicit() || Ctor->isCopyOrMoveConstructor()) {
+    if (Ctor->isExplicit() && Ctor->isCopyOrMoveConstructor()) {
+      auto isKWExplicit = [](const Token &Tok) {
+        return Tok.is(tok::raw_identifier) &&
+               StringRef(Tok.getRawIdentifierData(), Tok.getLength()) ==
+                   "explicit";
+      };
+      SourceRange ExplicitTokenRange =
+          FindToken(*Result.SourceManager, Result.Context->getLangOpts(),
+                    Ctor->getOuterLocStart(), Ctor->getLocEnd(), isKWExplicit);
+      if (ExplicitTokenRange.isValid()) {
+        DiagnosticBuilder Diag = diag(ExplicitTokenRange.getBegin(),
+                                      "Copy constructor declared explicit.");
+        Diag << FixItHint::CreateRemoval(
+            CharSourceRange::getCharRange(ExplicitTokenRange));
+      }
+    }
     return;
+  }
+
   if (Ctor->getNumParams() == 0 || Ctor->getMinRequiredArguments() > 1)
     return;
   SourceLocation Loc = Ctor->getLocation();
   diag(Loc, "Single-argument constructors must be explicit")
       << FixItHint::CreateInsertion(Loc, "explicit ");
 }
 
 class GoogleModule : public ClangTidyModule {

unittests/clang-tidy/GoogleModuleTest.cpp

 }
 
 TEST(ExplicitConstructorCheckTest, OutOfLineDefinitions) {
   EXPECT_EQ("class C { explicit C(int i); }; C::C(int i) {}",
             runCheckOnCode<ExplicitConstructorCheck>(
                 "class C { C(int i); }; C::C(int i) {}"));
 }
 
+TEST(ExplicitConstructorCheckTest, RemoveExplicit) {
+  EXPECT_EQ("class A { A(const A&); };\n"
+            "class B { /*asdf*/  B(const B&); };\n"
+            "class C { /*asdf*/  C(const C&); };",
+            runCheckOnCode<ExplicitConstructorCheck>(
+                "class A { explicit    A(const A&); };\n"
+                "class B { explicit   /*asdf*/  B(const B&); };\n"
+                "class C { explicit/*asdf*/  C(const C&); };"));
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang