[PATCH] D110925: [clangd] Follow-up on rGdea48079b90d

[Sam McCall via Phabricator via llvm-commits]

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