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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Looks good. Apart from some nitpicks, I'm happy with this. This isn't really my area so it'd be good to get an explicit lgtm from one of the reviewers.



================
Comment at: tools/clang-import-test/CMakeLists.txt:7
+  clang-import-test.cpp
+  )
+
----------------
@beanz recently went on a crusade to add dependencies to intrinsics_gen to a bunch of stuff. Chances are, this tool probably depends on intrinsics_gen, so I'd look into that and add the dep if needed.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:306
+  std::transform(ImportCIs.begin(), ImportCIs.end(), UnownedCIs.begin(),
+                 std::mem_fn(&std::unique_ptr<CompilerInstance>::get));
+  llvm::Expected<std::unique_ptr<CompilerInstance>> ExpressionCI =
----------------
This transform smells a little strange. I'd rather see `ArrayRef<std::unique_ptr<...>>` used everywhere, through a typedef/using-decl if necessary.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:310
+  if (auto E = ExpressionCI.takeError()) {
+    llvm::errs() << (llvm::toString(std::move(E)));
+    exit(-1);
----------------
There are redundant parens around your calls to toString(): I think these should be dropped.


Repository:
  rL LLVM

https://reviews.llvm.org/D27180