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
