[PATCH] D75665: [analyzer] On-demand parsing capability for CTU

[Gabor Marton via Phabricator via cfe-commits]

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