martong added a comment.
Hi Endre, just checked the latest update. Overall looks good to me, but found
some nits.
================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:260
+ /// that produce the AST used for analysis.
+ StringRef OnDemandParsingDatabase;
+
----------------
Should we rename this member to ....PathTo....InvocationList... ?
================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:581
+ SmallVector<const char *, 32> CommandLineArgs(InvocationCommand.size());
+ std::transform(InvocationCommand.begin(), InvocationCommand.end(),
+ CommandLineArgs.begin(),
----------------
Could we avoid this transfer if InvocationList was storing `const char *`
values instead of std::strings? Why can't we store `char*`s in InvocationList?
================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:593
+CrossTranslationUnitContext::ASTOnDemandLoader::lazyInitCompileCommands() {
+ /// Lazily initialize the invocation list filed used for on-demand parsing.
+ if (InvocationList)
----------------
typo: filed -> field
================
Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:6
+// 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
----------------
Perhaps we could document here that the compile_commands.json is needed ONLY
for %clang_extdef_map.
================
Comment at: clang/test/Analysis/ctu-on-demand-parsing.cpp:9
+// RUN: echo '[{"directory":"%t/Inputs","command":"clang++
ctu-chain.cpp","file":"ctu-chain.cpp"},{"directory":"%t/Inputs","command":"clang++
ctu-other.cpp","file":"ctu-other.cpp"}]' | sed -e 's/\\/\\\\/g' >
%t/compile_commands.json
+// RUN: echo '{"%t/Inputs/ctu-chain.cpp": ["g++", "%t/Inputs/ctu-chain.cpp"],
"%t/Inputs/ctu-other.cpp": ["g++", "%t/Inputs/ctu-other.cpp"]}' | sed -e
's/\\/\\\\/g' > %t/invocations.yaml
+// RUN: cd "%t" && %clang_extdef_map Inputs/ctu-chain.cpp Inputs/ctu-other.cpp
> externalDefMap.txt
----------------
I know this is just a nit, but this is very hard to read. Do you think we could
break this up (and other long lines) into multiple lines but within the same
RUN directive?
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
