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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Just nits left really.

In D110386#3020890 <https://reviews.llvm.org/D110386#3020890>, @tschuett wrote:

> Do you want to wrap the `unsigned` in a struct to make it slightly safer, but more complicated to use?

I like this idea, though it's probably not a huge deal either way.
(I marginally prefer `enum class HeaderID : unsigned {};` over a struct, but it's basically the same thing)



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1382
       llvm::StringMap<SourceParams> ProxSources;
-      for (auto &Entry : Includes.includeDepth(
-               SM.getFileEntryForID(SM.getMainFileID())->getName())) {
-        auto &Source = ProxSources[Entry.getKey()];
-        Source.Cost = Entry.getValue() * ProxOpts.IncludeCost;
-        // Symbols near our transitive includes are good, but only consider
-        // things in the same directory or below it. Otherwise there can be
-        // many false positives.
-        if (Entry.getValue() > 0)
-          Source.MaxUpTraversals = 1;
+      auto IncludeStructureID =
+          Includes.getID(SM.getFileEntryForID(SM.getMainFileID()));
----------------
this isn't the ID of the include structure, it's the ID of the main file


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1383
+      auto IncludeStructureID =
+          Includes.getID(SM.getFileEntryForID(SM.getMainFileID()));
+      if (IncludeStructureID) {
----------------
IncludeStructure is mutable here, I think it'd be safe to getOrCreate here.

Alternatively, make this infallible having IncludeStructure record the HeaderID for the main file in its constructor, and just use Includes.Root here.

Either way, I'd rather not deal with `Expected<>` error handling at runtime if we can define this error away.


================
Comment at: clang-tools-extra/clangd/Headers.h:116
 public:
-  std::vector<Inclusion> MainFileIncludes;
+  // Identifying files in a way that persists from preamble build to subsequent
+  // builds is surprisingly hard. FileID is unavailable in InclusionDirective(),
----------------
As mentioned offline, this is an implementation comment, but it doesn't say *what the HeaderID is*.

Suggest something like:
```
HeaderID identifies file in the include graph.
It corresponds to a FileEntry rather than a FileID, but stays stable across preamble & main file builds.
```

I'd move this impl comment back down to the private StringMap, but it could also follow the description.


================
Comment at: clang-tools-extra/clangd/Headers.h:119
+  // and RealPathName and UniqueID are not preserved in the preamble.
+  // We use the FileEntry::Name, which is stable, interned into a "file index".
+  // HeaderID is mapping the FileEntry::Name to the internal representation:
----------------
comment still refers to "file index" as if it were introducing a term we use elsewhere


================
Comment at: clang-tools-extra/clangd/Headers.h:125
+
+  llvm::Expected<HeaderID> getID(const FileEntry *Entry) const;
+  HeaderID getOrCreateID(const FileEntry *Entry);
----------------
I think this can be Optional rather than Expected - much more lightweight.


================
Comment at: clang-tools-extra/clangd/Headers.h:129
+  StringRef getRealPath(HeaderID ID) const {
+    return ID < RealPathNames.size() ? RealPathNames[ID] : StringRef();
+  }
----------------
this should be an assertion (or just leave that up to the vector) - passing an out-of-range HeaderID is invalid


================
Comment at: clang-tools-extra/clangd/Headers.h:138
   // Root --> 0, #included file --> 1, etc.
   // Root is clang's name for a file, which may not be absolute.
+  // Usually it should be
----------------
comment is out of date


================
Comment at: clang-tools-extra/clangd/Headers.h:140
+  // Usually it should be
+  // getFile(SM.getFileEntryForID(SM.getMainFileID())->getName()).
+  llvm::DenseMap<HeaderID, unsigned> includeDepth(HeaderID Root) const;
----------------
getName() is not right here.
(Hopefully the example will fit on the "Usually" line when fixed?)



================
Comment at: clang-tools-extra/clangd/Headers.h:144
+  // This updates IncludeChildren, but not MainFileIncludes.
+  void recordInclude(HeaderID Including, HeaderID Included);
+
----------------
we don't need this method if IncludeChildren is exposed, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386