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


================
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.
----------------
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...


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:7336
+
+TEST_P(ASTImporterOptionSpecificTestBase, ImportDeductionGuide) {
+  TranslationUnitDecl *FromTU = getTuDecl(
----------------
Does this test provide an assertion failure in the base?


================
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())));
----------------
Could you please formulate expectations (assertions) on the DeclContext's of the two template parameters? I'd expect them to be different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114418