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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/Headers.cpp:174
+  IncludeStructure::HeaderID Result = R.first->getValue();
+  auto RealPathName = Entry->tryGetRealPathName();
+  if (!RealPathName.empty() &&
----------------
no auto here. I really hope this isn't std::string :-)

Actually I think you can avoid extracting this variable at all, because the !RealPathName.empty() check is redundant. The case where both sides are empty is rare enough that it's fine to assign as long as the *stored* name is empty.


================
Comment at: clang-tools-extra/clangd/Headers.cpp:176
+  if (!RealPathName.empty() &&
+      RealPathNames[static_cast<unsigned>(Result)].empty())
+    RealPathNames[static_cast<unsigned>(Result)] = RealPathName.str();
----------------
extract `std::string &RealPathName = RealPathNames[...]`?


================
Comment at: clang-tools-extra/clangd/Headers.cpp:185
+  llvm::DenseMap<HeaderID, unsigned> Result;
+  if (static_cast<unsigned>(Root) >= RealPathNames.size()) {
+    elog("Requested includeDepth for {0} which doesn't exist IncludeStructure",
----------------
This is a "can't happen" condition - remove or replace with an assert?


================
Comment at: clang-tools-extra/clangd/Headers.cpp:205
           CurrentLevel.push_back(Child);
-          const auto &Name = RealPathNames[Child];
           // Can't include files if we don't have their real path.
+          if (!RealPathNames[static_cast<unsigned>(Child)].empty())
----------------
This is no longer true, we don't need the check.


================
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(),
----------------
sammccall wrote:
> 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.
This doesn't seem to be done.
You've added a line to the *bottom* of the implementation comment defining `HeaderID` in terms of the implementation.

A reader rather needs complete self-contained description of the concept at the *top*, optionally followed by an explanation about the implementation (separated by a blank line).


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:70
+  IncludeStructure::HeaderID getID(StringRef Filename) {
+    auto Includes = collectIncludes();
+    auto Entry = Clang->getSourceManager().getFileManager().getFile(Filename);
----------------
reparsing the code several times to obtain header IDs doesn't seem reasonable. It's surprising behavior from a test helper, and it requires that header IDs are stable across parses!

Can we just pass in the IncludeStructure instead?


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:234
+  auto Includes = collectIncludes();
+  EXPECT_THAT(Includes.IncludeChildren[getID(MainFile)],
+              UnorderedElementsAreArray({getID(FooHeader), getID(BarHeader)}));
----------------
Why are we asserting on every element of the map one at a time, instead of the whole map at once? Seems like it would be more regular and easier to read.

I'd probably just write:
```
DenseMap<HeaderID, SmallVector<HeaderID>> Expected = { ... };
EXPECT_EQ(Expected, Includes.IncludeChildren);
```


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:236
+              UnorderedElementsAreArray({getID(FooHeader), getID(BarHeader)}));
+  EXPECT_TRUE(Includes.IncludeChildren[getID(BarHeader)].empty());
+  EXPECT_THAT(Includes.IncludeChildren[getID(FooHeader)],
----------------
EXPECT_THAT(foo.empty()) -> EXPECT_THAT(foo, IsEmpty()).
(Much better error messages)


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:511
                   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap<unsigned> SM) {
+  auto Flatten = [](const llvm::DenseMap<IncludeStructure::HeaderID, unsigned>
+                        &IncludeDepth,
----------------
The ratio of setup to punchline of the rest of this test seems like it's gotten out of hand (it wasn't great before, but it's doubled).

What about just
```
auto &FM = PatchedAST->getSourceManager().getFileManager();
auto &Includes = PatchedAST->getIncludeStructure();
auto MainID = Includes.getID(FM.getFile(testPath("foo.cpp")));
auto AuxID = Includes.getID(FM.getFile(testPath("sub/aux.h")));
EXPECT_THAT(Includes.IncludeChildren.at(MainID), Contains(AuxID));
```

It tests a bit less I guess, but seems like enough


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:568
                   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap<unsigned> SM) {
+  // Ensure file proximity signals are correct.
+  auto Flatten = [](const llvm::DenseMap<IncludeStructure::HeaderID, unsigned>
----------------
and here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386