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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:227
+    /// Identifier.
+    virtual LoadResultTy load(StringRef Identifier) = 0;
+    virtual ~ASTLoader() = default;
----------------
xazax.hun wrote:
> 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.
> 
Well yeah, we could do that, it is a good idea, thanks! We will consider this in the future. I like in this idea that the command line options to Clang would be simplified. But then we must be transparent and show/log the user which method we are using.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665