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

[Sean Callanan via Phabricator via cfe-commits]

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

Thank you for your review, Vedant!  I will update the patch to reflect your comments in a moment.

> I'm concerned about the amount of covered-but-untested code this patch introduces. Since there are no CHECK lines, it's hard for me to verify that this tool is doing the right thing.

What you're observing is that this is very much a skeleton.  What it can do right now is let the parser know that a `struct S` exists, but not what its contents are.  That's why the test is so simple, too.  As soon as the lexical lookup machinery exists, we'll be able to add tests accessing fields etc. and make sure everything is copacetic.

> Along with this change, I suggest stripping out a fair amount of code for the initial commit (probably PrintSourceForLocation, and maybe anything related to LogLookups).

That's fair.  As a low-level testing tool I'd like to make sure that I have a logging mechanism later on that allows tests to verify that the compiler made the right queries during a parse; that said, I can add these functions back in when tests require them.



================
Comment at: tools/clang-import-test/clang-import-test.cpp:302
+bool Parse(const std::string &Path, std::unique_ptr<CompilerInstance> &CI,
+           llvm::ArrayRef<CompilerInstance *> Imports) {
+  CI = BuildCompilerInstance();
----------------
vsk wrote:
> I suggest making this `Expected<std::unique_ptr<CompilerInstance>> Parse(..., ArrayRef<std::unique_ptr<CompilerInstance>>)`. This way, there's no way to mistake CI for an input param, there's no need for an extra step to convert std::unique_ptr<CompilerInstance> to CompilerInstance *, and it's harder to drop an error from Parse without logging/handling it.
Ah yes, this is a pattern I hadn't internalized yet.  Thanks for the reminder.


Repository:
  rL LLVM

https://reviews.llvm.org/D27180