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

[Sean Callanan via Phabricator via cfe-commits]

spyffe marked 11 inline comments as done.
spyffe added a comment.

Thank you, Alex!  I've responded in a few places inline below, and will update this patch momentarily.



================
Comment at: tools/clang-import-test/clang-import-test.cpp:106
+
+    const char *LineEnd = nullptr;
+
----------------
a.sidorin wrote:
> 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";
> ```
> ?
I'm going to think about this a little more.  Personally I find the loop more readable.
That said StringRef instead of std::string seems an obvious win:
```
    llvm::StringRef LineString(LineBegin, LineEnd - LineBegin);
``` 


================
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;
----------------
a.sidorin wrote:
> Please add a newline here.
I just ran clang-format on the whole file.  Sorry for the noise.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:222
+                           SmallVectorImpl<Decl *> &Result) override {
+    
+  }
----------------
a.sidorin wrote:
> Extra spaces.
Sigh.  Yes, I had a preliminary implementation of this in mind, but decided to split that out into a separate patch.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:238
+
+  CompilerInvocation::CreateFromArgs(*Inv, ClangArgv.data(),
+                                     &ClangArgv.data()[ClangArgv.size()],
----------------
a.sidorin wrote:
> `ClangArgv.begin(), ClangArgv.end()`
```
.../llvm/tools/clang/tools/clang-import-test/clang-import-test.cpp:236:44: error: no viable conversion from 'iterator' (aka '__wrap_iter<const char **>') to 'const char *const *'
  CompilerInvocation::CreateFromArgs(*Inv, ClangArgv.begin(), ClangArgv.end(),
                                           ^~~~~~~~~~~~~~~~~
.../llvm/tools/clang/include/clang/Frontend/CompilerInvocation.h:139:49: note: passing argument to parameter 'ArgBegin' here
                             const char* const *ArgBegin,
                                                ^
```


Repository:
  rL LLVM

https://reviews.llvm.org/D27180