Fix assertion in google-explicit-constructor check when the constructor is defined in a macro.

[alexfh]

Summary

We shouldn't suggest replacements in macros anyway, as we can't see all
usages of the macro and ensure the replacement is safe for all of them.

[klimek]

clang-tidy/google/GoogleTidyModule.cpp

37

This seems a very conservative approach, as there seem to be many cases where we can provide a fix in a macro... Perhaps we can use Lexer::makeFileCharRange?

[alexfh]

This seems a very conservative approach, as there seem to be many cases where we can provide a fix in a macro...

Do you know a way to ensure that a change to a macro is safe? We don't want to remove "explicit" from a definition of a compatibility macro, for example ;)

Also, this case is quite rare. I've found only one instance internally. So maybe we shouldn't worry much about it.

Perhaps we can use Lexer::makeFileCharRange?

This could be used to avoid problems with other (possible, but extremely rare) corner cases.

clang-tidy/google/GoogleTidyModule.cpp

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 &)) {
+  if (StartLoc.isMacroID() || EndLoc.isMacroID())
+    return SourceRange();
   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 (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(),
-                                    "%0 constructor declared explicit.")
-                               << (Ctor->isMoveConstructor() ? "Move" : "Copy");
+    DiagnosticBuilder Diag =
+        diag(Ctor->getLocation(), "%0 constructor declared explicit.")
+    if (ExplicitTokenRange.isValid()) {
       Diag << FixItHint::CreateRemoval(
           CharSourceRange::getCharRange(ExplicitTokenRange));
     }
   }
 
   if (Ctor->isExplicit() || Ctor->isCopyOrMoveConstructor() ||
       Ctor->getNumParams() == 0 || Ctor->getMinRequiredArguments() > 1)
     return;

unittests/clang-tidy/GoogleModuleTest.cpp

EXPECT_EQ("class A { A(const A&); };\n"
             "class B { /*asdf*/  B(B&&); };\n"
             "class C { /*asdf*/  C(const C&, int i = 0); };",
             runCheckOnCode<ExplicitConstructorCheck>(
                 "class A { explicit    A(const A&); };\n"
                 "class B { explicit   /*asdf*/  B(B&&); };\n"
                 "class C { explicit/*asdf*/  C(const C&, int i = 0); };"));
 }
 
+TEST(ExplicitConstructorCheckTest, RemoveExplicitWithMacros) {
+  EXPECT_EQ(
+      "#define A(T) class T##Bar { explicit T##Bar(const T##Bar &b) {} };\n"
+      "A(Foo);",
+      runCheckOnCode<ExplicitConstructorCheck>(
+          "#define A(T) class T##Bar { explicit T##Bar(const T##Bar &b) {} };\n"
+          "A(Foo);"));
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang