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

[Vedant Kumar via Phabricator via cfe-commits]

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