[NFC] [Modules] Refactor ODR checking for default template argument in ASTReader

[ChuanqiXu]

Summary

This patch tries to refactor ODR checking process for default template argument in ASTReader. We could save about 100 lines of code after the refactoring. The patch is intended to be NFC.

Test Plan:
check-all

[ChuanqiXu]

gentle ping~

[urnathan]

Looks like a good cleanup. Let me know what you think about the idea of retaining the switch.

clang/lib/Serialization/ASTReader.cpp

10176–10184

Rather than use dyn_cast, can't you use the switch(getKind()) idiom of the original? That'll reduce the number of virtual calls won't it? Bonus points for passing getKind() into the lambda so the compiler has a chance of noticing a loop unswitching possibility?

[ChuanqiXu]

clang/lib/Serialization/ASTReader.cpp

10176–10184

dyn_cast is implemented as "isa<X>(Val) ? cast<X>(Val) : nullptr;". Maybe the judgement here is what you says 'virtual call', isn't it? So it looks like the switch version would be better. From the source code, the switch version contains one call to getKind() and at most 3 comparison with int. But the current version would contain 2 calls to isa<> and 3 calls to cast<> and 4 null check. So we might think the switch version is better.

But on the one hand, the compiler optimization should be able to eliminate the redundant calls in current version. So the number of getKind() and comparison should be able to be reduced. (I know there is specific optimization for switch, but the cases is too small so that there is no big optimization space.)

On the other hand, the switch version would require much more lines of code, a draft of it would be:

switch(D->getKind()) {
   case TemplateTypeParmDecl:
       auto *TTP = cast<TemplateTypeParmDecl>(D);
       return TTP->hasDefaultArgument() &&
                      !TTP->defaultArgumentWasInherited();
   case ....
}

I feel it is worse. And I can't believe the performance here would be significant. Premature optimization is the root of all evil.

For the loop switching optimization, as I said, there number of calls to getKind() wouldn't be larger. On the other hand, I don't think it is good to do loop switching here for the judgement of getKind()... It is not worthy. The loop is relatively big and the comparison is relatively cheap. It would pay more to do loop unstitching for this.

[urnathan]

ok, thanks for considering the suggestion.

clang/include/clang/Basic/DiagnosticSerializationKinds.td

def note_module_odr_violation_definition_data : Note <
   "%ordinal2 base class %3 with "
   "%select{public|protected|private|no}4 access specifier}1">;
 
 def err_module_odr_violation_template_parameter : Error <
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "unnamed template parameter|"
-  "template parameter %4|"
+  "template parameter %5|"
   "template parameter with %select{no |}4default argument|"
   "template parameter with default argument}3">;
 
-
 def note_module_odr_violation_template_parameter : Note <
   "but in '%0' found "
   "%select{"
   "unnamed template parameter %2|"
-  "template parameter %2|"
+  "template parameter %3|"
   "template parameter with %select{no |}2default argument|"
   "template parameter with different default argument}1">;
 
 def err_module_odr_violation_mismatch_decl : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{end of class|public access specifier|private access specifier|"
   "protected access specifier|static assert|field|method|type alias|typedef|"

clang/lib/Serialization/ASTReader.cpp

for (auto &RecordPair : Merge.second) {
       const ClassTemplateDecl *FirstTemplate =
           FirstRecord->getDescribedClassTemplate();
       const ClassTemplateDecl *SecondTemplate =
           SecondRecord->getDescribedClassTemplate();
 
       assert(!FirstTemplate == !SecondTemplate &&
              "Both pointers should be null or non-null");
 
-      enum ODRTemplateDifference {
-        ParamEmptyName,
-        ParamName,
-        ParamSingleDefaultArgument,
-        ParamDifferentDefaultArgument,
-      };
-
       if (FirstTemplate && SecondTemplate) {
         DeclHashes FirstTemplateHashes;
         DeclHashes SecondTemplateHashes;
 
         auto PopulateTemplateParameterHashs =
             [&ComputeSubDeclODRHash](DeclHashes &Hashes,
                                      const ClassTemplateDecl *TD) {
               for (auto *D : TD->getTemplateParameters()->asArray()) {
for (auto &RecordPair : Merge.second) {
 
         auto FirstIt = FirstTemplateHashes.begin();
         auto FirstEnd = FirstTemplateHashes.end();
         auto SecondIt = SecondTemplateHashes.begin();
         for (; FirstIt != FirstEnd; ++FirstIt, ++SecondIt) {
           if (FirstIt->second == SecondIt->second)
             continue;
 
-          auto ODRDiagTemplateError = [FirstRecord, &FirstModule, this](
-                                          SourceLocation Loc, SourceRange Range,
-                                          ODRTemplateDifference DiffType) {
-            return Diag(Loc, diag::err_module_odr_violation_template_parameter)
-                   << FirstRecord << FirstModule.empty() << FirstModule << Range
-                   << DiffType;
-          };
-          auto ODRDiagTemplateNote = [&SecondModule, this](
-                                         SourceLocation Loc, SourceRange Range,
-                                         ODRTemplateDifference DiffType) {
-            return Diag(Loc, diag::note_module_odr_violation_template_parameter)
-                   << SecondModule << Range << DiffType;
-          };
-
           const NamedDecl* FirstDecl = cast<NamedDecl>(FirstIt->first);
           const NamedDecl* SecondDecl = cast<NamedDecl>(SecondIt->first);
 
           assert(FirstDecl->getKind() == SecondDecl->getKind() &&
                  "Parameter Decl's should be the same kind.");
 
+          enum ODRTemplateDifference {
+            ParamEmptyName,
+            ParamName,
+            ParamSingleDefaultArgument,
+            ParamDifferentDefaultArgument,
+          };
+
+          auto hasDefaultArg = [](const NamedDecl *D) {
+            if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(D))
+              return TTP->hasDefaultArgument() &&
+                      !TTP->defaultArgumentWasInherited();
+            if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(D))
+              return NTTP->hasDefaultArgument() &&
+                      !NTTP->defaultArgumentWasInherited();
+            auto *TTP = cast<TemplateTemplateParmDecl>(D);
+            return TTP->hasDefaultArgument() &&
+                    !TTP->defaultArgumentWasInherited();
+          };
+          bool hasFirstArg = hasDefaultArg(FirstDecl);
+          bool hasSecondArg = hasDefaultArg(SecondDecl);
+
+          ODRTemplateDifference ErrDiffType;
+          ODRTemplateDifference NoteDiffType;
+
           DeclarationName FirstName = FirstDecl->getDeclName();
           DeclarationName SecondName = SecondDecl->getDeclName();
 
           if (FirstName != SecondName) {
-            const bool FirstNameEmpty =
+            bool FirstNameEmpty =
                 FirstName.isIdentifier() && !FirstName.getAsIdentifierInfo();
-            const bool SecondNameEmpty =
-                SecondName.isIdentifier() && !SecondName.getAsIdentifierInfo();
-            assert((!FirstNameEmpty || !SecondNameEmpty) &&
-                   "Both template parameters cannot be unnamed.");
-            ODRDiagTemplateError(FirstDecl->getLocation(),
-                                 FirstDecl->getSourceRange(),
-                                 FirstNameEmpty ? ParamEmptyName : ParamName)
-                << FirstName;
-            ODRDiagTemplateNote(SecondDecl->getLocation(),
-                                SecondDecl->getSourceRange(),
-                                SecondNameEmpty ? ParamEmptyName : ParamName)
-                << SecondName;
-            break;
-          }
-
-          switch (FirstDecl->getKind()) {
-          default:
-            llvm_unreachable("Invalid template parameter type.");
-          case Decl::TemplateTypeParm: {
-            const auto *FirstParam = cast<TemplateTypeParmDecl>(FirstDecl);
-            const auto *SecondParam = cast<TemplateTypeParmDecl>(SecondDecl);
-            const bool HasFirstDefaultArgument =
-                FirstParam->hasDefaultArgument() &&
-                !FirstParam->defaultArgumentWasInherited();
-            const bool HasSecondDefaultArgument =
-                SecondParam->hasDefaultArgument() &&
-                !SecondParam->defaultArgumentWasInherited();
-
-            if (HasFirstDefaultArgument != HasSecondDefaultArgument) {
-              ODRDiagTemplateError(FirstDecl->getLocation(),
-                                   FirstDecl->getSourceRange(),
-                                   ParamSingleDefaultArgument)
-                  << HasFirstDefaultArgument;
-              ODRDiagTemplateNote(SecondDecl->getLocation(),
-                                  SecondDecl->getSourceRange(),
-                                  ParamSingleDefaultArgument)
-                  << HasSecondDefaultArgument;
-              break;
-            }
-
-            assert(HasFirstDefaultArgument && HasSecondDefaultArgument &&
-                   "Expecting default arguments.");
-
-            ODRDiagTemplateError(FirstDecl->getLocation(),
-                                 FirstDecl->getSourceRange(),
-                                 ParamDifferentDefaultArgument);
-            ODRDiagTemplateNote(SecondDecl->getLocation(),
-                                SecondDecl->getSourceRange(),
-                                ParamDifferentDefaultArgument);
-
-            break;
-          }
-          case Decl::NonTypeTemplateParm: {
-            const auto *FirstParam = cast<NonTypeTemplateParmDecl>(FirstDecl);
-            const auto *SecondParam = cast<NonTypeTemplateParmDecl>(SecondDecl);
-            const bool HasFirstDefaultArgument =
-                FirstParam->hasDefaultArgument() &&
-                !FirstParam->defaultArgumentWasInherited();
-            const bool HasSecondDefaultArgument =
-                SecondParam->hasDefaultArgument() &&
-                !SecondParam->defaultArgumentWasInherited();
-
-            if (HasFirstDefaultArgument != HasSecondDefaultArgument) {
-              ODRDiagTemplateError(FirstDecl->getLocation(),
-                                   FirstDecl->getSourceRange(),
-                                   ParamSingleDefaultArgument)
-                  << HasFirstDefaultArgument;
-              ODRDiagTemplateNote(SecondDecl->getLocation(),
-                                  SecondDecl->getSourceRange(),
-                                  ParamSingleDefaultArgument)
-                  << HasSecondDefaultArgument;
-              break;
-            }
-
-            assert(HasFirstDefaultArgument && HasSecondDefaultArgument &&
-                   "Expecting default arguments.");
-
-            ODRDiagTemplateError(FirstDecl->getLocation(),
-                                 FirstDecl->getSourceRange(),
-                                 ParamDifferentDefaultArgument);
-            ODRDiagTemplateNote(SecondDecl->getLocation(),
-                                SecondDecl->getSourceRange(),
-                                ParamDifferentDefaultArgument);
-
-            break;
-          }
-          case Decl::TemplateTemplateParm: {
-            const auto *FirstParam = cast<TemplateTemplateParmDecl>(FirstDecl);
-            const auto *SecondParam =
-                cast<TemplateTemplateParmDecl>(SecondDecl);
-            const bool HasFirstDefaultArgument =
-                FirstParam->hasDefaultArgument() &&
-                !FirstParam->defaultArgumentWasInherited();
-            const bool HasSecondDefaultArgument =
-                SecondParam->hasDefaultArgument() &&
-                !SecondParam->defaultArgumentWasInherited();
-
-            if (HasFirstDefaultArgument != HasSecondDefaultArgument) {
-              ODRDiagTemplateError(FirstDecl->getLocation(),
-                                   FirstDecl->getSourceRange(),
-                                   ParamSingleDefaultArgument)
-                  << HasFirstDefaultArgument;
-              ODRDiagTemplateNote(SecondDecl->getLocation(),
-                                  SecondDecl->getSourceRange(),
-                                  ParamSingleDefaultArgument)
-                  << HasSecondDefaultArgument;
-              break;
-            }
-
-            assert(HasFirstDefaultArgument && HasSecondDefaultArgument &&
-                   "Expecting default arguments.");
-
-            ODRDiagTemplateError(FirstDecl->getLocation(),
-                                 FirstDecl->getSourceRange(),
-                                 ParamDifferentDefaultArgument);
-            ODRDiagTemplateNote(SecondDecl->getLocation(),
-                                SecondDecl->getSourceRange(),
-                                ParamDifferentDefaultArgument);
-
-            break;
-          }
-          }
+            bool SecondNameEmpty = SecondName.isIdentifier() &&
+                                    !SecondName.getAsIdentifierInfo();
+            ErrDiffType = FirstNameEmpty ? ParamEmptyName : ParamName;
+            NoteDiffType = SecondNameEmpty ? ParamEmptyName : ParamName;
+          } else if (hasFirstArg == hasSecondArg)
+            ErrDiffType = NoteDiffType = ParamDifferentDefaultArgument;
+          else
+            ErrDiffType = NoteDiffType = ParamSingleDefaultArgument;
 
+          Diag(FirstDecl->getLocation(),
+                diag::err_module_odr_violation_template_parameter)
+              << FirstRecord << FirstModule.empty() << FirstModule
+              << FirstDecl->getSourceRange() << ErrDiffType << hasFirstArg
+              << FirstName;
+          Diag(SecondDecl->getLocation(),
+                diag::note_module_odr_violation_template_parameter)
+              << SecondModule << SecondDecl->getSourceRange() << NoteDiffType
+              << hasSecondArg << SecondName;
           break;
         }
 
         if (FirstIt != FirstEnd) {
           Diagnosed = true;
           break;
         }
       }