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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:56
+  load_threshold_reached,
+  ambiguous_compile_commands_database
 };
----------------
The two enum values refer to compilation database and compile command database. I'd prefer to use the same wording in both values.


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:227
+    /// Identifier.
+    virtual LoadResultTy load(StringRef Identifier) = 0;
+    virtual ~ASTLoader() = default;
----------------
I am not sure if this is good design.
Here, if the meaning of the `Identifier` depends on the subclass, the caller of this method always needs to be aware of the dynamic type of the object. What is the point of a common base class if we always need to know the dynamic type?

Looking at the code this does not look bad. But it might be a code smell.


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:285
   public:
-    ASTUnitStorage(const CompilerInstance &CI);
+    ASTUnitStorage(CompilerInstance &CI);
     /// Loads an ASTUnit for a function.
----------------
Why is this no longer const?


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:120
+    case index_error_code::ambiguous_compile_commands_database:
+      return "Compile commands database contains multiple references to the "
+             "same source file.";
----------------
Unfortunately, this is a very common case for a large number of projects that supports multiple targets (e.g. 32 and 64 bits). Is there a plan to mitigate this problem?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75665/new/

https://reviews.llvm.org/D75665