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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to