[PATCH] D110925: [clangd] Follow-up on rGdea48079b90d
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 1 06:11:06 PDT 2021
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
More information about the cfe-commits
mailing list