martong added inline comments.
================ Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:56 + load_threshold_reached, + ambiguous_compile_commands_database }; ---------------- xazax.hun wrote: > The two enum values refer to compilation database and compile command > database. I'd prefer to use the same wording in both values. +1 ================ Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:227 + /// Identifier. + virtual LoadResultTy load(StringRef Identifier) = 0; + virtual ~ASTLoader() = default; ---------------- 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. ================ 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."; ---------------- xazax.hun wrote: > 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? I don't think we could do that. We need to merge ASTs of those TUs that are linkable. Otherwise the merge will be unsuccessful because of certain mismatches in Decls (structural eq will fail). Even in CodeChecker we are planning to issue a hard error in these ambiguous cases, even with the PCH method too. (The error would be suppressible by the user of course, but if that is suppressed then we take no responsibility.) 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
