xazax.hun added inline comments.
================ Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:56 + load_threshold_reached, + ambiguous_compile_commands_database }; ---------------- The two enum values refer to compilation database and compile command database. I'd prefer to use the same wording in both values. ================ Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:227 + /// Identifier. + virtual LoadResultTy load(StringRef Identifier) = 0; + virtual ~ASTLoader() = default; ---------------- 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. ================ Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:285 public: - ASTUnitStorage(const CompilerInstance &CI); + ASTUnitStorage(CompilerInstance &CI); /// Loads an ASTUnit for a function. ---------------- Why is this no longer const? ================ 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."; ---------------- 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? 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