martong added a comment. > Probably this is a bug in the AST creation but currently it works this way.
I don't think this is a bug, deduction guides have a convoluted implementation. See the related DeclContext issue with them when local typdefs are involved: https://reviews.llvm.org/D92209 ================ Comment at: clang/lib/AST/ASTImporter.cpp:6081 + // FunctionTemplateDecl objects are created, but in different order. In this + // way DeclContext of these template parameters may change relative to the + // "from" context. Because these DeclContext values look already not stable ---------------- balazske wrote: > martong wrote: > > ? > Probably better: > In this way the DeclContext of these template parameters is not necessary the > same as in the "from" context. okay, but necessary->necessarily ================ Comment at: clang/lib/AST/ASTImporter.cpp:6082-6085 + // "from" context. Because these DeclContext values look already not stable + // and unimportant this change looks acceptable. + // For these reasons the old DeclContext must be saved to change the lookup + // table later. ---------------- balazske wrote: > martong wrote: > > I think this sentence does not provide any meaningful information and does > > not increase the understand-ability. Plus the word `change` is overloaded, > > first I though you meant the patch itself... > It is still good to have an explanation of why the DeclContext is not exactly > preserved at import. And the DeclContext is really not "stable", not easily > predictable from the source code. Okay, then we could write "Consequently, the DeclContext of these Decls may change several times until the top-level import call is finished." ================ Comment at: clang/unittests/AST/ASTImporterTest.cpp:7336 + +TEST_P(ASTImporterOptionSpecificTestBase, ImportDeductionGuide) { + TranslationUnitDecl *FromTU = getTuDecl( ---------------- balazske wrote: > martong wrote: > > Does this test provide an assertion failure in the base? > We get the "trying to remove not contained Decl" in ASTImporterLookupTable. ok. ================ Comment at: clang/unittests/AST/ASTImporterTest.cpp:7348-7353 + // Get the implicit deduction guide for (non-default) constructor of 'B'. + auto *FromDG1 = FirstDeclMatcher<FunctionTemplateDecl>().match( + FromTU, functionTemplateDecl(templateParameterCountIs(3))); + // User defined deduction guide. + auto *FromDG2 = FirstDeclMatcher<CXXDeductionGuideDecl>().match( + FromTU, cxxDeductionGuideDecl(unless(isImplicit()))); ---------------- balazske wrote: > martong wrote: > > Could you please formulate expectations (assertions) on the DeclContext's > > of the two template parameters? I'd expect them to be different. > I left this out because the "instability" mentioned above. It is possible to > make the assertions for this exact code. We can better say that it comes from > a set of possible values, these are the current `DeductionGuideDecl`, or > another one for the same class, or the class itself (but I am not sure in > this any more). It is possible to get different value for different template > parameters of the same template. So, we could have ``` ASSERT_NE(cast<Decl>(FromDG1->getTemplateParameters()->getParam(0)->getDeclContext()), cast<Decl>(FromDG1->getTemplateParameters()->getParam(1)->getDeclContext())); ASSERT_NE(cast<Decl>(FromDG1->getTemplateParameters()->getParam(0)->getDeclContext()), cast<Decl>(FromDG1->getTemplateParameters()->getParam(2)->getDeclContext())); ASSERT_EQ(cast<Decl>(FromDG1->getTemplateParameters()->getParam(1)->getDeclContext()), cast<Decl>(FromDG1->getTemplateParameters()->getParam(2)->getDeclContext())) ``` And then after the `Import` calls we could have the same expectations on `ToDG1->getParam(0)` and the rest. Couldn't we? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114418/new/ https://reviews.llvm.org/D114418 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
