sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks! Just nits (This review addresses comments on https://reviews.llvm.org/rGdea48079b90d40f2087435b778544dffb0ab1793) ================ Comment at: clang-tools-extra/clangd/Headers.cpp:175 + if (Entry == MainFileEntry) { + RealPathNames[0] = MainFileEntry->tryGetRealPathName().str(); + return static_cast<IncludeStructure::HeaderID>(0u); ---------------- if RealPathNames.front().empty... ================ Comment at: clang-tools-extra/clangd/Headers.h:123 + void setMainFileEntry(const FileEntry *Entry) { + assert(Entry && Entry->isValid()); ---------------- This is a strange public member, and would need to be documented. But it seems better yet to make collectIncludeStructureCallback a member (`IncludeStructure::collect()`?) so we can just encapsulate this. ================ Comment at: clang-tools-extra/clangd/Headers.h:125 + assert(Entry && Entry->isValid()); + assert(!RealPathNames.empty()); + this->MainFileEntry = Entry; ---------------- nit: this assertion feels a little out-of-place/unneccesary ================ Comment at: clang-tools-extra/clangd/Headers.h:150 + llvm::DenseMap<HeaderID, unsigned> + includeDepth(HeaderID Root = static_cast<HeaderID>(0u)) const; ---------------- if we're going to expose the root headerID, we should do it in a named constant and reference that here. ================ Comment at: clang-tools-extra/clangd/Headers.h:158 private: + const FileEntry *MainFileEntry; + ---------------- this member should be documented (I think you can just move the bit about HeaderID(0) from below to here. ================ Comment at: clang-tools-extra/clangd/Headers.h:158 private: + const FileEntry *MainFileEntry; + ---------------- sammccall wrote: > this member should be documented (I think you can just move the bit about > HeaderID(0) from below to here. `= nullptr` ================ Comment at: clang-tools-extra/clangd/Headers.h:168 + // We reserve HeaderID(0) for the main file and will manually check for that + // in getID and getOrCreateID because llvm::sys::fs::UniqueID is not stable + // when their content of the main file changes. ---------------- nit: it's the *value* that's unstable, not the type. So just "the UniqueID" ================ Comment at: clang-tools-extra/clangd/Headers.h:169 + // in getID and getOrCreateID because llvm::sys::fs::UniqueID is not stable + // when their content of the main file changes. llvm::DenseMap<llvm::sys::fs::UniqueID, HeaderID> UIDToIndex; ---------------- their -> the ================ Comment at: llvm/include/llvm/Support/FileSystem/UniqueID.h:17 +#include "llvm/ADT/DenseMap.h" #include <cstdint> ---------------- you only need DenseMapInfo here Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110925/new/ https://reviews.llvm.org/D110925 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
