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

[Balázs Kéri via Phabricator via cfe-commits]

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:
> > > 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?
They can be (are in the current case) not all equal in the **From** context and can be different in the **To** context (related to the corresponding AST nodes in the **From** context). At least the code does not ensure that these will be the same. There was originally a FIXME at this place, I wanted to change it to this new comment. Because from semantic point of view it should not matter if the DeclContext of a template parameter is some implicit deduction guide or another one, probably both are correct (this is why I don't wanted to add assertion for exactly one case).


================
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:
> > > 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.
OK, I add the assertions for every DeclContext. If a future change (in Sema) results in different behavior the test will fail and needs to be updated. Without the assertions it will probably not fail and the test remains not updated for the new behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114418