[PATCH] D128704: [clang-extdef-mapping] Directly process .ast files

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks, this is a great addition!

- Please use Capitalized names for all the variables (as per the LLVM coding standards suggests here <https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly>).
- Could you please update the release notes part of Clang Static Analyzer as this is a cool user facing change.

> I tried this on llvm/lib/AsmParser/Parser.cpp and running extdef-mapping on the .cpp file took 5.4s on my machine. While running it on the .ast file it took 2s.

I am wondering if you could have a comparison on more files? I think a measurement run on all `clang` source/ast files comparing the overall run-time could be even more persuasive.



================
Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:124-129
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
+  TextDiagnosticPrinter *DiagClient =
+      new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
+  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags(
+      new DiagnosticsEngine(DiagID, &*DiagOpts, DiagClient));
----------------
Is there a way to reuse the `DiagOpts` (and the other Diag variables) over the multiple `HandleAST` calls? If not then we might use smart pointers to avoid leaking.


================
Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:142-143
+
+  std::unique_ptr<MapExtDefNamesConsumer> consumer =
+      std::make_unique<MapExtDefNamesConsumer>(unit->getASTContext(), absPath);
+  consumer->HandleTranslationUnit(unit->getASTContext());
----------------
Could this be a simple non-pointer variable?


================
Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:151
+                       CompilationDatabase &compilations) {
+  std::vector<std::string> sourceToBeParsed;
+  for (StringRef src : sourceFiles) {
----------------
I'd rather use a plural from here, there might be more than one source files.


================
Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:152
+  std::vector<std::string> sourceToBeParsed;
+  for (StringRef src : sourceFiles) {
+    if (src.endswith(".ast")) {
----------------
Could you please add a one-liner explanatory comment for this loop?


================
Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:175-177
   const char *Overview = "\nThis tool collects the USR name and location "
                          "of external definitions in the source files "
                          "(excluding headers).\n";
----------------
Could you please extend the documentation of the tool with the new behavior?


================
Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:122
+
+  CompilerInstance CI;
+
----------------
thieta wrote:
> Not sure if I can just create a compilerinstance here and if I should feed it anymore data then I already do.
I think it is okay to create the `CI` like you do. However, it would be enough to create that only once, not in every `HandleAST` call. Perhaps, it could be `static` or you might pass a reference to it from `main`.


================
Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:136
+  FileManager fm(CI.getFileSystemOpts());
+  SmallString<128> absPath(astPath);
+  fm.makeAbsolutePath(absPath);
----------------
thieta wrote:
> thieta wrote:
> > Pretty sure 128 is wrong here - but I searched the codebase and that seems to be the most common size used? I couldn't find anything using PATH_MAX or something like that.
> Never mind 128 is just the default size not the absolute size. Got it.
128 looks good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128704/new/

https://reviews.llvm.org/D128704