[PATCH] D114418: [clang][ASTImporter] Update lookup table correctly at deduction guides.

[Gabor Marton via Phabricator via cfe-commits]

martong 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.
----------------
balazske wrote:
> 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.
So, you say that is it possible that the DeclContext of the params are equal in the "from" context, but after the import we may end up having different DeclContexts for the imported params in the "to" context?


================
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:
> > 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.)
> An "unrelated" change in the source, for example different order of declarations, change in template-template parameters, may result in change of the DeclContext.)

I think having a different order of the declarations would be a different test case since the source code in the raw string literal would change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114418/new/

https://reviews.llvm.org/D114418