[PATCH] D38845: [ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr

Gabor Marton via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Apr 25 12:26:42 PDT 2018


martong added inline comments.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1556
+  // Converting code texts into TUs
+  std::transform(Codes.begin(), Codes.end(), std::back_inserter(FromTUs),
+                 [this](StringRef Code) {
----------------
Instead of having 4 `FromTUs`, perhaps a simpler alternative would be to have only one, which contains all the code snippets with `declToImport0` ... `declToImport3` (Or just skip declToImport since we no longer need that phabricated name, so we could just have a simple name like `X`) and importing them one by one later with `ASTImporterTestBase::Import`.
This would spare the need for the transform and the for cycle below.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1569
+        FirstDeclMatcher<FunctionTemplateDecl>().match(*TB, Pattern);
+    if (!FromDSDRE)
+      return;
----------------
I think, this `if` would be needed only if the test suite would be parameterized also with `ArgVector{"-fdelayed-template-parsing"}`, but that is not.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1573
+    auto To = cast<FunctionTemplateDecl>(Import(FromDSDRE, Lang_CXX));
+    EXPECT_TRUE(FirstDeclMatcher<FunctionTemplateDecl>().match(To, Pattern));
+  }
----------------
It will be hard to notice based on the test output which DSDRE's import was unsuccessful. Therefore I'd unroll the for loop instead, also please see the above comment that it would be more practical to have one FromTU.


https://reviews.llvm.org/D38845





More information about the cfe-commits mailing list