[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    [PATCH] D27180: Testbed and skeleton of a new expression parser
From:       Vedant Kumar via Phabricator via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2016-11-30 22:10:04
Message-ID: fdf773e0dc07e65265e0d444255226d4 () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]