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

[Gabor Marton via Phabricator via cfe-commits]

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