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

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hello Gabor,

Sorry for the delay with review: the patch is pretty big and it was hard to find time for reviewing it.
I like the patch but it requires some cleanup like formatting changes. Could you please clang-format new code?
Also, I'd like to avoid code duplication. I pointed some places where it happens.



================
Comment at: unittests/AST/ASTImporterTest.cpp:167
+
+  // We may have several From context and related translation units.  In each
+  // AST, the buffers for the source are handled via references and are set
----------------
context_s_; two spaces before 'In'.


================
Comment at: unittests/AST/ASTImporterTest.cpp:178
+  // already exists then the file will not be created again as a duplicate.
+  void createVirtualFile(StringRef FileName, const std::string &Code) {
+    assert(ToAST);
----------------
Could you refactor `testImport()` to use `createVirtualFile()` to avoid code duplication?


================
Comment at: unittests/AST/ASTImporterTest.cpp:194
+  // of the identifier into the To context.
+  // Must not call more than once within the same test.
+  std::tuple<Decl *, Decl *>
----------------
Must not be called?


================
Comment at: unittests/AST/ASTImporterTest.cpp:197
+  getImportedDecl(StringRef FromSrcCode, Language FromLang, StringRef ToSrcCode,
+                  Language ToLang, StringRef Identifier = "declToImport") {
+    ArgVector FromArgs = getBasicRunOptionsForLanguage(FromLang),
----------------
This magic constant was factored out in my review.


================
Comment at: unittests/AST/ASTImporterTest.cpp:198
+                  Language ToLang, StringRef Identifier = "declToImport") {
+    ArgVector FromArgs = getBasicRunOptionsForLanguage(FromLang),
+              ToArgs = getBasicRunOptionsForLanguage(ToLang);
----------------
This means that testing under MSVC is omitted. It caused a lot of buildbot headache before, so I strongly recommend to add it.


================
Comment at: unittests/AST/ASTImporterTest.cpp:202
+    FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs);
+    TU &FromTu = FromTUs.back();
+
----------------
FromTU?


================
Comment at: unittests/AST/ASTImporterTest.cpp:226
+
+    Decl *Imported = Importer.Import(*FoundDecls.begin());
+    assert(Imported);
----------------
`FoundDecls.front()`


================
Comment at: unittests/AST/ASTImporterTest.cpp:978
+    R"(
+template<typename _T>
+struct X {};
----------------
Could you please format inline code?


================
Comment at: unittests/AST/ASTImporterTest.cpp:1090
+      R"(
+        template<class T>
+        class Base {};
----------------
Sometimes we start raw literal from the beginning of the line, sometimes - with indent. Is there any common style?


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


================
Comment at: unittests/AST/ASTImporterTest.cpp:1254
+  std::tie(From, To) = getImportedDecl(
+    R"(
+    void declToImport() {}
----------------
This can be written on single line.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1365
+
+  // There must be only one imported FunctionDecl ...
+  EXPECT_TRUE(FirstDeclMatcher<FunctionDecl>().match(ToTU, Pattern) ==
----------------
It looks like this piece of code duplicates below several times. If so, I think it should be factored into a separate function.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1443
+       ImportDefinitionThenPrototype) {
+  auto Pattern = functionDecl(hasName("f"));
+
----------------
This `Pattern` is repeated often.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1494
+  EXPECT_TRUE(!ProtoD->doesThisDeclarationHaveABody());
+  FunctionDecl* DefinitionD = LastDeclMatcher<FunctionDecl>().match(ToTU, Pattern);
+  EXPECT_TRUE(DefinitionD->doesThisDeclarationHaveABody());
----------------
FunctionDecl *DefinitionD (same upper).


================
Comment at: unittests/AST/ASTImporterTest.cpp:1526
+
+TEST_F(ImportFunctions,
+       OverriddenMethodsShouldBeImported) {
----------------
This fits 80-char limit, no need to split.


================
Comment at: unittests/AST/DeclMatcher.h:33
+  template <typename MatcherType>
+  NodeType *match(const Decl *D, const MatcherType &AMatcher) {
+    MatchFinder Finder;
----------------
We can use `ASTContext &` directly instead of `const Decl *`.


================
Comment at: unittests/AST/DeclMatcher.h:48
+class DeclCounter : public MatchFinder::MatchCallback {
+  unsigned count = 0;
+  void run(const MatchFinder::MatchResult &Result) override {
----------------
Member names should start with capital: `Count`.


================
Comment at: unittests/AST/DeclMatcher.h:59
+    MatchFinder Finder;
+    Finder.addMatcher(AMatcher.bind(""), this);
+    Finder.matchAST(D->getASTContext());
----------------
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);`


Repository:
  rC Clang

https://reviews.llvm.org/D43967