sammccall added a comment. Thanks, this mostly LG now. I'd consider splitting out the new infra change (IncludeStructure) from the feature change (include-cleaner treatment). In case the latter causes some problems...
================ Comment at: clang-tools-extra/clangd/Headers.cpp:92 InBuiltinFile = false; + // At the file exit time HeaderSearchInfo is valid and can be used to + // determine whether the file was a self-contained header or not. ---------------- nit: at the file exit time -> at file exit time ================ Comment at: clang-tools-extra/clangd/Headers.h:65 SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User; + bool SelfContained = true; llvm::Optional<unsigned> HeaderID; ---------------- Why redundantly track this on Inclusion? It's already available via IncludeStructure.SelfContained.contains(*HeaderID) ================ Comment at: clang-tools-extra/clangd/Headers.h:128 // populates the structure. - std::unique_ptr<PPCallbacks> collect(const SourceManager &SM); + std::unique_ptr<PPCallbacks> collect(const SourceManager &SM, + HeaderSearch &HeaderInfo); ---------------- Maybe it's neater just to pass the CompilerInstance& - there's no other practical way to use this. ================ Comment at: clang-tools-extra/clangd/Headers.h:158 + // Contains HeaderIDs of all self-contained entries in the IncludeStructure. + llvm::DenseSet<HeaderID> SelfContained; + ---------------- I think i'd prefer to see this private with an `isSelfContained` accessor, because the representation isn't the only sensible one. Case in point: almost all headers in practice are self-contained, so in fact storing the set of non-self-contained headers seems preferable. ================ Comment at: clang-tools-extra/clangd/SourceCode.cpp:53 +namespace { + ---------------- these helpers are 1000 lines away from the only place they're used, please move them closer ================ Comment at: clang-tools-extra/clangd/SourceCode.h:330 +/// guard, does not rely on preprocessor state). This will read a portion of the +/// file which might be discouraged in performance-sensitive context. +bool isSelfContainedHeader(const FileEntry *FE, FileID ID, ---------------- This still doesn't really describe the situation: - if you're not getting the header from a preamble, it won't have to read anything as it'll be in-memory already - if you are, then the fact that the content may have changed is at least as big a problem - the string scanning itself is not ideal to do redundantly from a performance POV I'd rather say: "This scans source code, and should not be called when using a preamble. Prefer to access the cache in IncludeStructure::isSelfContained if you can." Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114370/new/ https://reviews.llvm.org/D114370 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits