[clang] [clang][ASTImporter] New fix for default template parameter values. (PR #101836)

[Matheus Izvekov via cfe-commits]

=?utf-8?q?Balázs_Kéri?= <balazs.keri at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/101836 at github.com>


================
@@ -5968,11 +5962,21 @@ ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   }
 
   if (D->hasDefaultArgument()) {
+    // Default argument can be "inherited" when it has a reference to the
+    // previous declaration (of the default argument) which is stored only once.
+    // Here we import the default argument in any case, and the inherited state
+    // is updated later after the parent template was created. If the
+    // inherited-from object would be imported here it causes more difficulties
+    // (parent template may not be created yet and import loops can occur).
     Expected<TemplateArgumentLoc> ToDefaultArgOrErr =
         import(D->getDefaultArgument());
     if (!ToDefaultArgOrErr)
       return ToDefaultArgOrErr.takeError();
-    ToD->setDefaultArgument(ToD->getASTContext(), *ToDefaultArgOrErr);
+    // The just called import process can trigger import of the parent template
+    // which can update the default argument value to "inherited". This should
+    // not be changed.
+    if (!ToD->hasDefaultArgument())
+      ToD->setDefaultArgument(ToD->getASTContext(), *ToDefaultArgOrErr);
----------------
mizvekov wrote:

I think this avoiding to set a default argument can prevent diagnosing ODR violations, ie different values for the default argument.

In any case, I don't see how it would help to import the default argument, but not set it. The argument would just end up discarded.

The cycle issue we usually solve by deferring importation. I think what could maybe work here is importing all default arguments separately after we are done with the rest of the template.

One thing that was missing from this implementation, and seems to be leading to the wrong source locations issue, is to import the inheritedFrom state.

Ie call `getInheritedFrom` on `D`, import that returned declaration into the `To` context, and set it with `setInherited` on `ToD`.

https://github.com/llvm/llvm-project/pull/101836