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