[PATCH] D43967: [ASTImporter] Add test helper Fixture

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: unittests/AST/ASTImporterTest.cpp:198
+                  Language ToLang, StringRef Identifier = "declToImport") {
+    ArgVector FromArgs = getBasicRunOptionsForLanguage(FromLang),
+              ToArgs = getBasicRunOptionsForLanguage(ToLang);
----------------
a.sidorin wrote:
> This means that testing under MSVC is omitted. It caused a lot of buildbot headache before, so I strongly recommend to add it.
Good point. Changed all tests to be parameterized on the language options.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1090
+      R"(
+        template<class T>
+        class Base {};
----------------
a.sidorin wrote:
> Sometimes we start raw literal from the beginning of the line, sometimes - with indent. Is there any common style?
Yes you are right it is not consistent. I changed everywhere to follow a common style, to start with an indent.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1193
+
+  auto Check = [](Decl *D) -> bool {
+    std::array<const char*, 3> FieldNamesInOrder{{"a", "b", "c"}};
----------------
a.sidorin wrote:
> Code duplication with upper example. In my patch, I introduced a matcher for this; I think it can be reused here.
Agree, the new matcher of yours makes this far simpler, changed to use that.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1365
+
+  // There must be only one imported FunctionDecl ...
+  EXPECT_TRUE(FirstDeclMatcher<FunctionDecl>().match(ToTU, Pattern) ==
----------------
a.sidorin wrote:
> It looks like this piece of code duplicates below several times. If so, I think it should be factored into a separate function.
I have refactored the repeating part by using `DeclCounter`, because what we actually want to ensure is that there is only one decl.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1443
+       ImportDefinitionThenPrototype) {
+  auto Pattern = functionDecl(hasName("f"));
+
----------------
a.sidorin wrote:
> This `Pattern` is repeated often.
I don't see why is that a problem, could you please elaborate?


================
Comment at: unittests/AST/DeclMatcher.h:33
+  template <typename MatcherType>
+  NodeType *match(const Decl *D, const MatcherType &AMatcher) {
+    MatchFinder Finder;
----------------
a.sidorin wrote:
> We can use `ASTContext &` directly instead of `const Decl *`.
This `DeclMatcher` ought to be a generic purpose matcher which might be used outside of `ASTImporterTest.cpp` as well.
The `D` as a parameter provides the generality.
Perhaps if we used this only inside `ASTImporterTestBase` then we could add another overload which uses the ASTContext... but which one, the "to" or the "from".


================
Comment at: unittests/AST/DeclMatcher.h:59
+    MatchFinder Finder;
+    Finder.addMatcher(AMatcher.bind(""), this);
+    Finder.matchAST(D->getASTContext());
----------------
a.sidorin wrote:
> This will cause confusing miscompile if `AMatcher` is not a `BindableMatcher` - like matchers defined with `AST_MATCHER`-like macros, for example. Isn't better to write this call like this:
> `Finder.addMatcher(BindableMatcher<NodeType>(AMatcher).bind(""), this);`
I am not sure if this could work. Actually there is no conversion from the BindableMatcher to a DeclarationMatcher.
```
../../git/llvm/tools/clang/unittests/AST/DeclMatcher.h:37:12: error: no matching member function for call to 'addMatcher'
    Finder.addMatcher(internal::BindableMatcher<NodeType>(AMatcher).bind(""),
    ~~~~~~~^~~~~~~~~~
../../git/llvm/tools/clang/unittests/AST/ASTImporterTest.cpp:1554:57: note: in instantiation of function template specialization 'clang::ast_matchers::DeclMatcher<clang::CXXMethodDecl, clang::ast_matchers::DeclMatcherKind::Last>::match<clang::ast_matchers::internal::BindableMatcher<clang::Decl> >' requested here
  CXXMethodDecl *Def = LastDeclMatcher<CXXMethodDecl>().match(FromTU, Pattern);
                                                        ^
```


Repository:
  rC Clang

https://reviews.llvm.org/D43967