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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
