[PATCH] D110386: [clangd] Refactor IncludeStructure: use File (unsigned) for most computations

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Headers.h:115
 public:
+  using File = unsigned;
+
----------------
this concept needs documentation.


================
Comment at: clang-tools-extra/clangd/Headers.h:115
 public:
+  using File = unsigned;
+
----------------
sammccall wrote:
> this concept needs documentation.
there's a naming issue here:
 - "File" is an overloaded enough concept that you still end up referring to these as "file indexes" below, which isn't clear.
 - obviously this integer isn't either the file itself nor our record of it, so the metaphor is a bit confusing

What do you think about just `ID`, which would be spelled from the outside as `IncludeStructure::ID`?
Maybe `HeaderID` is better. I don't think it's a big problem that we have one for the main file too...




================
Comment at: clang-tools-extra/clangd/Headers.h:117
+
+  llvm::Optional<File> getFile(StringRef Name) const {
+    auto It = NameToIndex.find(Name);
----------------
"Name" certainly needs to be clarified here.
Do you have any use cases in mind where you wouldn't have a FileEntry on hand? That would make this typesafe (and in particular avoid passing absolute filenames, or strings like "utility" or "<utility>)


================
Comment at: clang-tools-extra/clangd/Headers.h:124
+
+  llvm::ArrayRef<File> getIncludedFiles(File Index) const {
+    auto It = IncludeChildren.find(Index);
----------------
it's unclear from this patch whether you want to expose the whole graph or just allow querying per-file - I doubt we need both.


================
Comment at: clang-tools-extra/clangd/Headers.h:124
+
+  llvm::ArrayRef<File> getIncludedFiles(File Index) const {
+    auto It = IncludeChildren.find(Index);
----------------
sammccall wrote:
> it's unclear from this patch whether you want to expose the whole graph or just allow querying per-file - I doubt we need both.
tests?


================
Comment at: clang-tools-extra/clangd/Headers.h:131
+
   std::vector<Inclusion> MainFileIncludes;
 
----------------
this public member is now sandwiched between methods


================
Comment at: clang-tools-extra/clangd/Headers.h:141
   // Usually it should be SM.getFileEntryForID(SM.getMainFileID())->getName().
-  llvm::StringMap<unsigned> includeDepth(llvm::StringRef Root) const;
+  llvm::StringMap<unsigned> includeDepth(StringRef Name) const;
 
----------------
The public interface now uses the File abstraction, so this method sticks out. (It has an awkward interface in the first place due to the need to use strings)

Suggest replacing it with `DenseMap<File, unsigned> includeDepth(File Root)`. We'd need to add `StringRef RealPath(File)` for the caller, but that seems to make sense anyway.


================
Comment at: clang-tools-extra/clangd/Headers.h:148
 
+  using AbstractIncludeGraph = llvm::DenseMap<File, SmallVector<File>>;
+
----------------
this isn't abstract :-)

also the public nature of it isn't used, in this patch at least.


================
Comment at: clang-tools-extra/clangd/Headers.h:151
 private:
+  File getOrCreatefileIndex(llvm::StringRef Name);
+
----------------
File should be capitalized.

FWIW, for a private method I prefer the old name here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110386/new/

https://reviews.llvm.org/D110386