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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
