[clang-tidy] Fix for cppcoreguidelines-prefer-member-initializer to handle classes declared in macros

[martong]

Summary

cppcoreguidelines-prefer-member-initializer crashes on classes declared inside macros (see bug 47778). This patch fixes this issue.

[alexfh]

Thanks for the fix! However, I'm not sure it's possible to correctly rewrite code in all cases where macros are involved. See a couple of motivating examples in the comment.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

498

Could you add tests where the field name comes from a macro argument and from token pasting? Something along the lines of:

#define MACRO4(field) struct InMacro1 { int field; InMacro1() { field = 0; } }

MACRO4(q);

#define MACRO5(X) X

MACRO5(struct InMacro1 { int field; InMacro1() { field = 0; } });

#define MACRO6(field) struct InMacro1 { int qqq ## field; InMacro1() { qqq ## field = 0; } }

MACRO6(q);

[alexfh]

clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp

169

Looks like clang-format is not happy with a number of places in the file. Could you git clang-format the patch? (Or just clang-format the whole file - maybe in a separate commit - if you don't have the git integration handy.)

[baloghadamsoftware]

Tests added, code reformatted.

[baloghadamsoftware]

In D89380#2330076, @alexfh wrote:

Thanks for the fix! However, I'm not sure it's possible to correctly rewrite code in all cases where macros are involved. See a couple of motivating examples in the comment.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

498

It seems that handling of macro parameters in the SourceManager is totally off. The location in these cases are the macro call itself, but the spelling location is not the real location inside the macro, but in MACRO4 it is the location of the argument still in the macro call. The case with MACRO6 is even worse, because its spelling location is erroneous. So I could only added some fixmes. However, it does not crash, at least. That is the main intention of this particular patch.

[alexfh]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

498

It's impossible to correctly handle replacements in all cases involving macros (apart from expanding all macros while applying fixes ;). The usual strategy is to warn always, but only suggest replacements when we can be sufficiently confident in their validity. In this case we can either disable fixes when any part of the code being touched is in a macro or try to detect that the whole range being modified is sort of "on the same level of macro hell". The Lexer::makeFileCharRange is supposed to help with the latter (see clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp for an example of using it).

[baloghadamsoftware]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

498

Actually, this is what the patch does: it does not suggest a fix in complex macro cases, only in the simplest ones. And most importantly, it prevents the crash mentioned in the Bugzilla bug report.

[alexfh]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

6

"results in ..."

6

The reason is that token pasting is used in the macro. There is no place in any of the existing source files where the text of the token that is created by token pasting exists (verbatim and contiguously).

7

"being"?

500

Please add CHECK-FIXES: for the macro usage (unchanged, I suppose?). Same below.

514–515

Do we need these disabled check lines? If so, please mention the reason in a comment.

Same below.

[martong]

@baloghadamsoftware Ping.

[whisperity]

In D89380#2913664, @martong wrote:

@baloghadamsoftware Ping.

Bug 47778 is marked FIXED by D97132.

[martong]

In D89380#2913750, @whisperity wrote:

In D89380#2913664, @martong wrote:

@baloghadamsoftware Ping.

Bug 47778 is marked FIXED by D97132.

Okay, thanks! So, I am going to abandon/close this.

clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp

void PreferMemberInitializerCheck::check(
   const auto *Body = cast<CompoundStmt>(Ctor->getBody());
 
   const CXXRecordDecl *Class = Ctor->getParent();
   SourceLocation InsertPos;
   bool FirstToCtorInits = true;
 
   for (const Stmt *S : Body->body()) {
     if (S->getBeginLoc().isMacroID()) {
-      StringRef MacroName =
-        Lexer::getImmediateMacroName(S->getBeginLoc(), *Result.SourceManager,
-                                     getLangOpts());
+      StringRef MacroName = Lexer::getImmediateMacroName(
+          S->getBeginLoc(), *Result.SourceManager, getLangOpts());
       if (MacroName.contains_lower("assert"))
         return;
     }
     if (isControlStatement(S))
       return;
 
     if (isNoReturnCallStatement(S))
       return;
 
     if (const auto *CondOp = dyn_cast<ConditionalOperator>(S)) {
       if (isNoReturnCallStatement(CondOp->getLHS()) ||
           isNoReturnCallStatement(CondOp->getRHS()))
         return;
     }
 
     const FieldDecl *Field;
     const Expr *InitValue;
     std::tie(Field, InitValue) = isAssignmentToMemberOf(Class, S);
-    if (Field) {
-      if (IsUseDefaultMemberInitEnabled && getLangOpts().CPlusPlus11 &&
-          Ctor->isDefaultConstructor() &&
-          (getLangOpts().CPlusPlus20 || !Field->isBitField()) &&
-          (!isa<RecordDecl>(Class->getDeclContext()) ||
-           !cast<RecordDecl>(Class->getDeclContext())->isUnion()) &&
-          shouldBeDefaultMemberInitializer(InitValue)) {
-        auto Diag =
-            diag(S->getBeginLoc(), "%0 should be initialized in an in-class"
-                                   " default member initializer")
+    if (!Field)
+      continue;
+    assert(InitValue && "An assigment to a field must also have an assigned"
+                        " value");
+
+    SourceLocation BeginLoc = S->getBeginLoc();
+    SourceLocation EndLoc = S->getEndLoc();
+    SourceRange InitValueRange = InitValue->getSourceRange();
+
+    if (BeginLoc.isMacroID()) {
+      BeginLoc = Result.SourceManager->getSpellingLoc(BeginLoc);
+      EndLoc = Result.SourceManager->getSpellingLoc(EndLoc);
+      InitValueRange = SourceRange(
+          Result.SourceManager->getSpellingLoc(InitValueRange.getBegin()),
+          Result.SourceManager->getSpellingLoc(InitValueRange.getEnd()));
+    }
+
+      auto Diag = diag(BeginLoc, "%0 should be initialized in an in-class"
                   << Field;
 
-        SourceLocation FieldEnd =
-            Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0,
-                                       *Result.SourceManager, getLangOpts());
-        Diag << FixItHint::CreateInsertion(FieldEnd,
-                                           UseAssignment ? " = " : "{")
-             << FixItHint::CreateInsertionFromRange(
-                    FieldEnd,
-                    CharSourceRange(InitValue->getSourceRange(), true))
-             << FixItHint::CreateInsertion(FieldEnd, UseAssignment ? "" : "}");
+      Diag << FixItHint::CreateInsertion(FieldEnd, UseAssignment ? " = " : "{")
+                  FieldEnd, CharSourceRange(InitValueRange, true))
 
-        SourceLocation SemiColonEnd =
-            Lexer::findNextToken(S->getEndLoc(), *Result.SourceManager,
-                                 getLangOpts())
-                ->getEndLoc();
-        CharSourceRange StmtRange =
-            CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
+          Lexer::findNextToken(EndLoc, *Result.SourceManager, getLangOpts())
+          CharSourceRange::getCharRange(BeginLoc, SemiColonEnd);
 
-        Diag << FixItHint::CreateRemoval(StmtRange);
-      } else {
-        auto Diag =
-            diag(S->getBeginLoc(), "%0 should be initialized in a member"
-                                   " initializer of the constructor")
-            << Field;
+      auto Diag = diag(BeginLoc, "'%0' should be initialized in a member"
+                  << Field->getName();
 
-        bool AddComma = false;
-        if (!Ctor->getNumCtorInitializers() && FirstToCtorInits) {
-          SourceLocation BodyPos = Ctor->getBody()->getBeginLoc();
-          SourceLocation NextPos = Ctor->getBeginLoc();
-          do {
-            InsertPos = NextPos;
-            NextPos = Lexer::findNextToken(NextPos, *Result.SourceManager,
-                                           getLangOpts())
-                          ->getLocation();
-          } while (NextPos != BodyPos);
-          InsertPos = Lexer::getLocForEndOfToken(
-              InsertPos, 0, *Result.SourceManager, getLangOpts());
+        if (BodyPos.isMacroID())
+          BodyPos = Result.SourceManager->getSpellingLoc(BodyPos);
+        if (NextPos.isMacroID())
+          NextPos = Result.SourceManager->getSpellingLoc(NextPos);
 
-          Diag << FixItHint::CreateInsertion(InsertPos, " : ");
-        } else {
-          bool Found = false;
-          for (const auto *Init : Ctor->inits()) {
-            if (Init->isMemberInitializer()) {
-              if (Result.SourceManager->isBeforeInTranslationUnit(
-                      Field->getLocation(), Init->getMember()->getLocation())) {
-                InsertPos = Init->getSourceLocation();
-                Found = true;
-                break;
-              }
-            }
-          }
+            SourceLocation InitLoc = Init->getSourceLocation();
+            if (InitLoc.isMacroID())
+              InitLoc = Result.SourceManager->getSpellingLoc(InitLoc);
+            SourceLocation InitMemberLoc = Init->getMember()->getLocation();
+            if (InitMemberLoc.isMacroID())
+              InitMemberLoc =
+                  Result.SourceManager->getSpellingLoc(InitMemberLoc);
+            SourceLocation FieldLoc = Field->getLocation();
+            if (FieldLoc.isMacroID())
+              FieldLoc = Result.SourceManager->getSpellingLoc(FieldLoc);
+                    FieldLoc, InitMemberLoc)) {
+              InsertPos = InitLoc;
 
-          if (!Found) {
-            if (Ctor->getNumCtorInitializers()) {
-              InsertPos = Lexer::getLocForEndOfToken(
-                  (*Ctor->init_rbegin())->getSourceRange().getEnd(), 0,
-                  *Result.SourceManager, getLangOpts());
-            }
-            Diag << FixItHint::CreateInsertion(InsertPos, ", ");
-          } else {
-            AddComma = true;
-          }
-        }
-        Diag << FixItHint::CreateInsertion(InsertPos, Field->getName())
-             << FixItHint::CreateInsertion(InsertPos, "(")
-             << FixItHint::CreateInsertionFromRange(
-                    InsertPos,
-                    CharSourceRange(InitValue->getSourceRange(), true))
-             << FixItHint::CreateInsertion(InsertPos, ")");
-        if (AddComma)
-          Diag << FixItHint::CreateInsertion(InsertPos, ", ");
+            SourceLocation LastInitEnd =
+                (*Ctor->init_rbegin())->getSourceRange().getEnd();
+            if (LastInitEnd.isMacroID())
+              LastInitEnd = Result.SourceManager->getSpellingLoc(LastInitEnd);
+                LastInitEnd, 0, *Result.SourceManager, getLangOpts());
+
+                  InsertPos, CharSourceRange(InitValueRange, true))
 
-        SourceLocation SemiColonEnd =
-            Lexer::findNextToken(S->getEndLoc(), *Result.SourceManager,
-                                 getLangOpts())
-                ->getEndLoc();
-        CharSourceRange StmtRange =
-            CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
+          Lexer::findNextToken(EndLoc, *Result.SourceManager, getLangOpts())
+          CharSourceRange::getCharRange(BeginLoc, SemiColonEnd);
 
-        Diag << FixItHint::CreateRemoval(StmtRange);
-        FirstToCtorInits = false;
-      }
-    }
-  }
-}
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-prefer-member-initializer %t -- -- -fcxx-exceptions
 
+// CHECK-MESSAGES: warning: 'qqqq' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
+// FIXME: The source location of the assignment in MACRO6 (see towards the end
+//        of this test file) is the macro itself and the spelling location is
+//        <scratch space>:6:1 for some strange reason. This results this warning
+//        to be placed at the very beginning of the message list.
+
 extern void __assert_fail (__const char *__assertion, __const char *__file,
     unsigned int __line, __const char *__function)
      __attribute__ ((__noreturn__));
 #define assert(expr) \
   ((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
 
 class Simple1 {
   int n;
Outside::Outside() {
     // CHECK-FIXES: Outside::Outside() : n(1), x(1.0) {
   n = 1;
     // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'n' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
     // CHECK-FIXES: {{^\ *$}}
   x = 1.0;
     // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'x' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
     // CHECK-FIXES: {{^\ *$}}
 }
+
+#define MACRO1 struct InMacro1 { int i; InMacro1() { i = 0; } };
+// CHECK-MESSAGES: :[[@LINE-1]]:54: warning: 'i' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
+// CHECK-FIXES: #define MACRO1 struct InMacro1 { int i; InMacro1() : i(0) { } };
+MACRO1
+
+#define MACRO2 struct InMacro2 { int i, j; InMacro2() : i(0) { j = 1; } };
+// CHECK-MESSAGES: :[[@LINE-1]]:64: warning: 'j' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
+// CHECK-FIXES: #define MACRO2 struct InMacro2 { int i, j; InMacro2() : i(0), j(1) { } };
+MACRO2
+
+#define MACRO3 struct InMacro3 { int i, j; InMacro3() : j(1) { i = 0; } };
+// CHECK-MESSAGES: :[[@LINE-1]]:64: warning: 'i' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
+// CHECK-FIXES: #define MACRO3 struct InMacro3 { int i, j; InMacro3() : i(0), j(1) { } };
+MACRO3
+
+#define MACRO4(field) struct InMacro4 { int field; InMacro4() { field = 0; } };
+// C_HECK-MESSAGES: :[[@LINE-1]]:65: warning: 'field' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
+// C_HECK-FIXES: #define MACRO4(field) struct InMacro4 { int field; InMacro4() : field(0) { } };
+MACRO4(q)
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'q' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
+// FIXME: The source location of the assignment is the macro itself and the
+//        spelling location is the argument in the macro call, instead of the
+//        place where it is used inside the macro.
+
+#define MACRO5(X) X
+MACRO5(struct InMacro5 { int field; InMacro5() { field = 0; } };)
+// CHECK-MESSAGES: :[[@LINE-1]]:50: warning: 'field' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
+// CHECK-FIXES: MACRO5(struct InMacro5 { int field; InMacro5() : field(0) { } };)
+
+#define MACRO6(field) struct InMacro6 { int qqq ## field; InMacro6() { \
+  qqq ## field = 0; } };
+// C_HECK-MESSAGES: :[[@LINE-1]]:3: warning: 'qqq ## field' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
+// C_HECK-FIXES: #define MACRO6(field) struct InMacro6 { int qqq ### field; InMacro6() : qqq ## field(0) { \
+// C_HECK-FIXES: } };
+MACRO6(q)