martong added inline comments.
================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:365 + llvm::SmallString<256> AbsPath = CTUDir; + llvm::sys::path::append(AbsPath, Identifier); ---------------- Could we check somehow if `CTUDir` is indeed an absolute path? If not then probably we should bail out with an error. Though I am not sure if there is an easy and portable way in llvm:: to check for beeing an absolute path. ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:375 +/// Load the AST from a source-file, which is supposed to be located inside the +/// compilation database \p OnDemandParsingCommands. The compilation database +/// can contain the path of the file under the key "file" as an absolute path, ---------------- There is no variable named as `OnDemandParsingCommands`, perhaps you made a rename but forgot to rename in the comments? ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:384 +/// the invocation inside ClangTool is always made with an absolute path. \p +/// ASTSourcePath is assumed to be the lookup-name of the file, which comes from +/// the Index. The Index is built by the \p clang-extdef-mapping tool, which is ---------------- There is no variable named as `ASTSourcePath`, perhaps you made a rename but forgot to rename in the comments? ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:413 + Files.push_back(std::string(Identifier)); + ClangTool Tool(*CompileCommands, Files, CI.getPCHContainerOperations()); + ---------------- martong wrote: > martong wrote: > > Seems like `Tool` has it's lifetime ended when `load()` finishes. But we > > have long living `ASTUnits` whose lifetime are longer then the `Tool` which > > created them. Is this not a problem? > `CI.getPCHContainerOperations()` is probably not needed, because we are not > going to exercise the ASTReader, so this could be a nullptr (default param > value). If `CI.getPCHContainerOperations()` is not needed then we can remove the `CI` member of `ASTOnDemandLoader`. ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:443 + /// be the absolute path of the file. + *SourceFilePath = Identifier.str(); + ---------------- Perhaps we should make sure that `Identifier` is indeed an absolute path and if not then we should emit an error. If a user do not use CodeChecker to generate the ExternalDefMapping it may contain relative paths. See e.g.: https://clang.llvm.org/docs/analyzer/user-docs/CrossTranslationUnit.html#manual-ctu-analysis ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:457 + return llvm::make_error<IndexError>( + index_error_code::ambiguous_compile_commands_database); + ---------------- Could we have a lit test for this case? I.e having a compile_commands.json with two ambiguous entries for the same file and then we should expect the compiler diag? (Note, I am asking this because it is not immediate for me from the code that this will ever fire.) ================ Comment at: clang/test/Analysis/ctu-main.c:53 clang_analyzer_eval(res == 6); // expected-warning{{TRUE}} + // Call something with uninitialized from the same function in which the implicit was called. + // This is necessary to reproduce a special bug in NoStoreFuncVisitor. ---------------- Is this hunk related? ================ Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:3 // RUN: mkdir -p %t/ctudir2 -// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu \ -// RUN: -emit-pch -o %t/ctudir2/ctu-other.c.ast %S/Inputs/ctu-other.c -// RUN: cp %S/Inputs/ctu-other.c.externalDefMap.txt %t/ctudir2/externalDefMap.txt -// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze \ +// RUN: echo '[{"directory":"%S/Inputs","command":"clang -x c -std=c89 -c ctu-other.c","file":"ctu-other.c"}]' | sed -e 's/\\/\\\\/g' > %t/ctudir2/compile_commands.json +// RUN: %clang_extdef_map %S/Inputs/ctu-other.c > %t/ctudir2/externalDefMap.txt ---------------- Why do we need `sed` here? I don't see any `\` in the compile commands json. Is it because of Windows builds? Could you please explain in a comment maybe in the previous line. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75665/new/ https://reviews.llvm.org/D75665 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
