whisperity added a comment. Please check the summary of the patch, it seems to contain old information as well.
================ Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:210-212 +Preferably the same compilation database should be used when generating the external definitions, and +during analysis. The analysis invocation must be provided with the directory which contains the mapping +files, and the compilation database which is used to determine compiler flags. ---------------- This is obsolete information. ================ Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:234 + public: + explicit ASTFileLoader(CompilerInstance &CI, StringRef CTUDir); + ---------------- Is the `explicit` needed here? ================ Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:388-389 + "parsing there is no need for pre-dumping ASTs. External " + "definition mapping is still needed, and a valid compilation " + "database with compile commands for the external TUs is also " + "necessary. Disabled by default.", ---------------- "valid compilation database" is this line still what we wish to say? A YAML is used, so most likely we need a "valid invocation list" or something. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:393-398 +ANALYZER_OPTION( + StringRef, CTUInvocationList, "ctu-invocation-list", + "The path to the YAML format file containing a mapping from source file " + "paths to command-line invocations represented as a list of arguments. " + "This invocation is used produce the source-file's AST.", + "invocations.yaml") ---------------- Maybe it would be worthwhile to add a dummy example like `main.cpp: g++ hello-world.cpp` which has the proper format into this help. ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:23-24 #include "clang/Index/USRGeneration.h" -#include "llvm/ADT/Triple.h" +#include "clang/Tooling/JSONCompilationDatabase.h" +#include "clang/Tooling/Tooling.h" +#include "llvm/ADT/Optional.h" ---------------- The idea here was to not depend on libtooling but these lines are still here! ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:34 #include <fstream> +#include <llvm/Option/ArgList.h> #include <sstream> ---------------- <> -> "" and order ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:122-123 return "Load threshold reached"; + case index_error_code::ambiguous_compilation_database: + return "Compilation database contains multiple references to the same " + "source file."; ---------------- still using terms "compilation database", is this intended? ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:583 + CommandLineArgs.begin(), + [](auto &&CmdPart) { return CmdPart.c_str(); }); + ---------------- balazske wrote: > Is here a special reason to use `auto &&` type (and not `auto &` or > `std::string &`)? Probably this code is faulty: The `CmdPart` is moved out > and then destructed, before the content of it (where the `c_str` points to) > is used? @balazske This is a generic lambda where `auto&&` is the universal reference. It is equivalent to writing `template <typename T> f(T&& param)`. It obeys the usual point of instantiation and reference cascading rules, if a `const std::string&` is given as `T`, `const std::string& &&` will become a single `&`. ================ Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:609 + llvm::SourceMgr SM; + llvm::yaml::Stream InvocationFiles(*ContentBuffer, SM); + ---------------- Are the headers from which these types come from included? ================ Comment at: clang/test/Analysis/Inputs/ctu-other.c:34-38 +// TODO: Support the GNU extension asm keyword as well. +// Example using the GNU extension: asm("mov $42, %0" : "=r"(res)); int inlineAsm() { int res; + __asm__("mov $42, %0" ---------------- Possibly unrelated change? ================ Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:1-15 +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: cp "%s" "%t/ctu-on-demand-parsing.c" +// RUN: cp "%S/Inputs/ctu-other.c" "%t/ctu-other.c" +// Path substitutions on Windows platform could contain backslashes. These are escaped in the json file. +// RUN: echo '[{"directory":"%t","command":"gcc -std=c89 -Wno-visibility ctu-other.c","file":"ctu-other.c"}]' | sed -e 's/\\/\\\\/g' > %t/compile_commands.json +// RUN: echo '"%t/ctu-other.c": ["gcc", "-std=c89", "-Wno-visibility", "ctu-other.c"]' | sed -e 's/\\/\\\\/g' > %t/invocations.yaml ---------------- Does the LIT syntax allow for better organising these commands? This is now a bit hard to read Something like this would help: ``` // RUN: rm -rf %t && mkdir -p %t // RUN: cp "%s" "%t/ctu-on-demand-parsing.c" && cp "%S/Inputs/ctu-other.c" "%t/ctu-other.c" // [empty] // [comment] // RUN: [json magic] // RUN: [yaml magic] // [empty] // RUN: extdef map // [empty] // RUN: cc1 ``` 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