balazske added inline comments.
================ 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. ---------------- martong wrote: > 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." The meaning of this is still different from what my original intent was: The DeclContext in the "To" context is different from the "From", even after the top-level import call. I wanted to emphasize here that this change of DeclContext after (top-level) import has no relevance, should cause no problems. ================ 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()))); ---------------- martong wrote: > 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? Probably at the "explicit" template parameters (the ones not coming from the class template) the DeclContext is always the deduction guide, so an `ASSERT_EQ(FromDG1->getTemplateParameters()->getParam(1)->getDeclContext(), FromDG1->getTemplatedDecl())` do work (for param 2 similar). I would not say that param 0 has always necessary a different DeclContext than the others, in this one case yes but not generally. This is why I do not like an assert for that. (Such an assert would not point out an invariant that is true always in the AST: An "unrelated" change in the source, for example different order of declarations, change in template-template parameters, may result in change of the DeclContext.) 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
