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

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

xazax.hun accepted this revision.
xazax.hun added a comment.

Ok, looks good to me.

The minor nit regarding the naming is easy to fix before commit. The design question I had is not a blocker, my suggested alternative can be implemented later (if desired) in a backward-compatible way from the user's point of view.



================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:227
+    /// Identifier.
+    virtual LoadResultTy load(StringRef Identifier) = 0;
+    virtual ~ASTLoader() = default;
----------------
martong wrote:
> xazax.hun wrote:
> > 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.
> The way how we process the `extDefMapping` file is identical in both cases. That's an index, keyed with the `USR`s of functions and then we get back a value. And the way how we use that value is different. In the PCH case that holds the path for the `.ast` file, in the ODM case that is the name of the source file which we must find in the compile db. So, I think the process of getting the AST for a USR requires the polymorphic behavior from the loaders.
> 
> We discussed other alternatives with Endre. We were thinking that maybe the `extDefMapping` file should be identical in both cases. But then we would need to add the `.ast` postfixes for the entries in the PCH case. And we cannot just do that, because we may not know if what is the correct postfix. The user may have generated `.pch` files instead. Also, we don't want to compel any Clang user to use CodeChecker (CC will always create `.ast` files). CTU should be running fine by manually executing the independent steps.
Let me rephrase my concerns a bit. Do we really need a polymorphic `ASTLoader` to be present for the whole analysis? Wouldn't it make more sense to always do the same thing, i.e. if we are given a pch file load it, if we are given a source file, parse it? This way we would not be restricted to on-demand or two pass ctu analysis, but we could do any combination of the two.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665