[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:
> > > 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