[PATCH] D27180: Testbed and skeleton of a new expression parser

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

That's excellent. Thank you for this work, Sean!



================
Comment at: tools/clang-import-test/CMakeLists.txt:19
+  )
\ No newline at end of file

----------------
Please add a newline here.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:106
+
+    const char *LineEnd = nullptr;
+
----------------
How about something like this:
```
StringRef Remain(LineBegin, Buffer->getBufferEnd() - LineBegin);
size_t EndPos = Remain.find_first_of("\r\n");
StringRef Line = (EndPos == StringRef::npos) ? Remain : StringRef(LineBegin, EndPos);
llvm::errs() << Line << "\n";
llvm::errs().indent(LocColumn) << "^\n";
```
?


================
Comment at: tools/clang-import-test/clang-import-test.cpp:115
+
+    fprintf(stderr, "%s\n", LineString.c_str());
+    std::string Space(LocColumn, ' ');
----------------
Why do we use `fprintf` instead of `llvm::errs()`?


================
Comment at: tools/clang-import-test/clang-import-test.cpp:130
+
+      SmallVector<char, 16> DiagText;
+      Info.FormatDiagnostic(DiagText);
----------------
SmallString? So, you will not need to push_back.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:143
+        if (!Invalid) {
+          llvm::errs() << Ref.str().c_str() << '\n';
+        }
----------------
No need in `c_str()` here.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:152
+class TestExternalASTSource : public ExternalASTSource {
+private:  llvm::ArrayRef<CompilerInstance *> ImportCIs;
+  std::map<CompilerInstance *, std::unique_ptr<ASTImporter>> ForwardImporters;
----------------
Please add a newline here.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:162
+      const bool MinimalImport = true;
+      ForwardImporters.emplace(std::make_pair(
+          ImportCI,
----------------
`ForwardImporters[ImportCI] = ...llvm::make_unique<>`?


================
Comment at: tools/clang-import-test/clang-import-test.cpp:179
+                                      DeclarationName Name) override {
+    std::vector<NamedDecl *> Decls;
+
----------------
SmallVector?


================
Comment at: tools/clang-import-test/clang-import-test.cpp:181
+
+    if (llvm::isa<TranslationUnitDecl>(DC)) {
+      for (CompilerInstance *I : ImportCIs) {
----------------
In LLVM code, qualifiers for llvm-style casts are not commonly used.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:222
+                           SmallVectorImpl<Decl *> &Result) override {
+    
+  }
----------------
Extra spaces.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:236
+  std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+                 [](std::string &s) -> const char * { return s.data(); });
+
----------------
`[](const std::string &s)`


================
Comment at: tools/clang-import-test/clang-import-test.cpp:238
+
+  CompilerInvocation::CreateFromArgs(*Inv, ClangArgv.data(),
+                                     &ClangArgv.data()[ClangArgv.size()],
----------------
`ClangArgv.begin(), ClangArgv.end()`


================
Comment at: tools/clang-import-test/clang-import-test.cpp:327
+}
+}
+
----------------
// end namespace


Repository:
  rL LLVM

https://reviews.llvm.org/D27180