vsk added a comment.
Thanks for working on this! I'm happy with the direction of this patch. It
makes sense that clang would have a tool to help test AST reconstruction from
'external' sources. As you pointed out, it provides a good avenue for clients
of the clang AST's to get better test coverage for their custom serialization
formats. I haven't been paying much attention to the discussion about the
clangDebuggerSupport library, but it sounds like your work will ultimately
depend on it. Is that correct?
I've left some lower-level comments inline.
At a higher level, 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 we really want to test
right away is that the tool can "dump" the correct definition for `struct S`
(since this implies that the importing went OK). A good way to go about this
would be to add a hidden "debug" cl::opt, dump all imported struct decls when
in -debug mode, and then add some CHECK lines for the expected struct decl.
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).
================
Comment at: tools/clang-import-test/clang-import-test.cpp:238
+
+ CompilerInvocation::CreateFromArgs(*Inv, ClangArgv.data(),
+ &ClangArgv.data()[ClangArgv.size()],
----------------
spyffe wrote:
> 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,
> ^
> ```
This should resolve it, but it seems less readable: `&*args.cbegin(),
&*args.cend()`. I'd leave this as-is..
================
Comment at: tools/clang-import-test/clang-import-test.cpp:164
+ ExpressionCI.getASTContext(), ExpressionCI.getFileManager(),
+ ImportCI->getASTContext(), ImportCI->getFileManager(),
MinimalImport);
+ ReverseImporters[ImportCI] = llvm::make_unique<ASTImporter>(
----------------
I think it's more idiomatic to say `/*MinimalImport=*/true` in clang.
================
Comment at: tools/clang-import-test/clang-import-test.cpp:282
+ llvm::LLVMContext &LLVMCtx) {
+ std::string ModuleName("$__module");
+ return std::unique_ptr<CodeGenerator>(CreateLLVMCodeGen(
----------------
This might as well be constructed as a StringRef from the get-go, since it's
eventually converted into one.
================
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();
----------------
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.
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
