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