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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Reverted in 36dc5c048ac745ef62769dd824a9a13659afc2ef 
 for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Looks like this breaks tests on Windows: http://45.33.8.238/win/45971/step_9.txt

PTAL!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0b1eff1bc5d0: [clangd] Refactor IncludeStructure: use File 
(unsigned) for most computations (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -44,9 +44,11 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAreArray;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast(arg))
@@ -493,7 +495,7 @@
   auto EmptyPreamble =
   buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
   ASSERT_TRUE(EmptyPreamble);
-  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, testing::IsEmpty());
+  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty());
 
   // Now build an AST using empty preamble and ensure patched includes worked.
   TU.Code = ModifiedContents.str();
@@ -507,18 +509,17 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto  = PatchedAST->getSourceManager().getFileManager();
+  // Copy so that we can use operator[] to get the children.
+  IncludeStructure Includes = PatchedAST->getIncludeStructure();
+  auto MainFE = FM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(MainFE);
+  auto MainID = Includes.getID(*MainFE);
+  auto AuxFE = FM.getFile(testPath("sub/aux.h"));
+  ASSERT_TRUE(AuxFE);
+  auto AuxID = Includes.getID(*AuxFE);
+  EXPECT_THAT(Includes.IncludeChildren[*MainID], Contains(AuxID));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -551,18 +552,20 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto  = ExpectedAST.getSourceManager().getFileManager();
+  // Copy so that we can getOrCreateID().
+  IncludeStructure Includes = ExpectedAST.getIncludeStructure();
+  auto MainFE = FM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(MainFE);
+  auto MainID = Includes.getOrCreateID(*MainFE);
+  auto  = PatchedAST->getSourceManager().getFileManager();
+  IncludeStructure PatchedIncludes = PatchedAST->getIncludeStructure();
+  auto PatchedMainFE = PatchedFM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedMainFE);
+  auto PatchedMainID = PatchedIncludes.getOrCreateID(*PatchedMainFE);
+  EXPECT_EQ(Includes.includeDepth(MainID)[MainID],
+PatchedIncludes.includeDepth(PatchedMainID)[PatchedMainID]);
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -29,8 +30,10 @@
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 375277.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.

Resolve the last comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -44,9 +44,11 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAreArray;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast(arg))
@@ -493,7 +495,7 @@
   auto EmptyPreamble =
   buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
   ASSERT_TRUE(EmptyPreamble);
-  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, testing::IsEmpty());
+  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty());
 
   // Now build an AST using empty preamble and ensure patched includes worked.
   TU.Code = ModifiedContents.str();
@@ -507,18 +509,17 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto  = PatchedAST->getSourceManager().getFileManager();
+  // Copy so that we can use operator[] to get the children.
+  IncludeStructure Includes = PatchedAST->getIncludeStructure();
+  auto MainFE = FM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(MainFE);
+  auto MainID = Includes.getID(*MainFE);
+  auto AuxFE = FM.getFile(testPath("sub/aux.h"));
+  ASSERT_TRUE(AuxFE);
+  auto AuxID = Includes.getID(*AuxFE);
+  EXPECT_THAT(Includes.IncludeChildren[*MainID], Contains(AuxID));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -551,18 +552,20 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto  = ExpectedAST.getSourceManager().getFileManager();
+  // Copy so that we can getOrCreateID().
+  IncludeStructure Includes = ExpectedAST.getIncludeStructure();
+  auto MainFE = FM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(MainFE);
+  auto MainID = Includes.getOrCreateID(*MainFE);
+  auto  = PatchedAST->getSourceManager().getFileManager();
+  IncludeStructure PatchedIncludes = PatchedAST->getIncludeStructure();
+  auto PatchedMainFE = PatchedFM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedMainFE);
+  auto PatchedMainID = PatchedIncludes.getOrCreateID(*PatchedMainFE);
+  EXPECT_EQ(Includes.includeDepth(MainID)[MainID],
+PatchedIncludes.includeDepth(PatchedMainID)[PatchedMainID]);
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -29,8 +30,10 @@
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedElementsAreArray;
 
 class HeadersTest : public ::testing::Test {
 public:
@@ -64,8 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:234
+  auto Includes = collectIncludes();
+  EXPECT_THAT(Includes.IncludeChildren[getID(MainFile)],
+  UnorderedElementsAreArray({getID(FooHeader), getID(BarHeader)}));

sammccall wrote:
> kbobyrev wrote:
> > sammccall wrote:
> > > 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> Expected = { ... };
> > > EXPECT_EQ(Expected, Includes.IncludeChildren);
> > > ```
> > This would expect the elements in the map to be in a particular order, 
> > isn't this something we don't want?
> `==` on DenseMap doesn't consider order.
> 
> If you mean the order within map values (i.e. lists of child edges): these 
> are in the order the `#includes` appear in the file, which seems fine to 
> depend on to me
Yes, I initially meant the `SmallVector` order, but yeah, this seems 
reasonable, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/Headers.cpp:205
   CurrentLevel.push_back(Child);
-  const auto  = RealPathNames[Child];
   // Can't include files if we don't have their real path.
+  if (!RealPathNames[static_cast(Child)].empty())

sammccall wrote:
> kbobyrev wrote:
> > sammccall wrote:
> > > This is no longer true, we don't need the check.
> > Wait, why not? We still have unresolved includes, e.g. preamble patches are 
> > like that, their `RealPathName`s stay empty.
> Right, but why do we need to filter them out here?
> Can't we just drop them when we use them to seed the proximity sources?
I feel like producing them in the first place is kind of redundant, what signal 
does it give to the user through such API?

Getting some "hidden"/"unresolved" includes certainly feels rather confusing to 
me, post-filtering outside probably requires understanding of the internals 
which does not look like a good idea. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 375253.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.

Switch to full DenseMap comparison in the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -44,9 +44,11 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAreArray;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast(arg))
@@ -493,7 +495,7 @@
   auto EmptyPreamble =
   buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
   ASSERT_TRUE(EmptyPreamble);
-  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, testing::IsEmpty());
+  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty());
 
   // Now build an AST using empty preamble and ensure patched includes worked.
   TU.Code = ModifiedContents.str();
@@ -507,18 +509,17 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto  = PatchedAST->getSourceManager().getFileManager();
+  // Copy so that we can use operator[] to get the children.
+  IncludeStructure Includes = PatchedAST->getIncludeStructure();
+  auto MainFE = FM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(MainFE);
+  auto MainID = Includes.getID(*MainFE);
+  auto AuxFE = FM.getFile(testPath("sub/aux.h"));
+  ASSERT_TRUE(AuxFE);
+  auto AuxID = Includes.getID(*AuxFE);
+  EXPECT_THAT(Includes.IncludeChildren[*MainID], Contains(AuxID));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -551,18 +552,20 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto  = ExpectedAST.getSourceManager().getFileManager();
+  // Copy so that we can getOrCreateID().
+  IncludeStructure Includes = ExpectedAST.getIncludeStructure();
+  auto MainFE = FM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(MainFE);
+  auto MainID = Includes.getOrCreateID(*MainFE);
+  auto  = PatchedAST->getSourceManager().getFileManager();
+  IncludeStructure PatchedIncludes = PatchedAST->getIncludeStructure();
+  auto PatchedMainFE = PatchedFM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedMainFE);
+  auto PatchedMainID = PatchedIncludes.getOrCreateID(*PatchedMainFE);
+  EXPECT_EQ(Includes.includeDepth(MainID),
+PatchedIncludes.includeDepth(PatchedMainID));
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -29,8 +30,10 @@
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedElementsAreArray;
 
 class HeadersTest : public ::testing::Test {
 public:
@@ 

[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.cpp:205
   CurrentLevel.push_back(Child);
-  const auto  = RealPathNames[Child];
   // Can't include files if we don't have their real path.
+  if (!RealPathNames[static_cast(Child)].empty())

kbobyrev wrote:
> sammccall wrote:
> > This is no longer true, we don't need the check.
> Wait, why not? We still have unresolved includes, e.g. preamble patches are 
> like that, their `RealPathName`s stay empty.
Right, but why do we need to filter them out here?
Can't we just drop them when we use them to seed the proximity sources?



Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:234
+  auto Includes = collectIncludes();
+  EXPECT_THAT(Includes.IncludeChildren[getID(MainFile)],
+  UnorderedElementsAreArray({getID(FooHeader), getID(BarHeader)}));

kbobyrev wrote:
> sammccall wrote:
> > 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> Expected = { ... };
> > EXPECT_EQ(Expected, Includes.IncludeChildren);
> > ```
> This would expect the elements in the map to be in a particular order, isn't 
> this something we don't want?
`==` on DenseMap doesn't consider order.

If you mean the order within map values (i.e. lists of child edges): these are 
in the order the `#includes` appear in the file, which seems fine to depend on 
to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/Headers.cpp:205
   CurrentLevel.push_back(Child);
-  const auto  = RealPathNames[Child];
   // Can't include files if we don't have their real path.
+  if (!RealPathNames[static_cast(Child)].empty())

sammccall wrote:
> This is no longer true, we don't need the check.
Wait, why not? We still have unresolved includes, e.g. preamble patches are 
like that, their `RealPathName`s stay empty.



Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:234
+  auto Includes = collectIncludes();
+  EXPECT_THAT(Includes.IncludeChildren[getID(MainFile)],
+  UnorderedElementsAreArray({getID(FooHeader), getID(BarHeader)}));

sammccall wrote:
> 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> Expected = { ... };
> EXPECT_EQ(Expected, Includes.IncludeChildren);
> ```
This would expect the elements in the map to be in a particular order, isn't 
this something we don't want?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 375237.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.

Resolve all but two comments, leave the clarification questions for these two.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -44,9 +44,11 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAreArray;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast(arg))
@@ -493,7 +495,7 @@
   auto EmptyPreamble =
   buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
   ASSERT_TRUE(EmptyPreamble);
-  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, testing::IsEmpty());
+  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty());
 
   // Now build an AST using empty preamble and ensure patched includes worked.
   TU.Code = ModifiedContents.str();
@@ -507,18 +509,17 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto  = PatchedAST->getSourceManager().getFileManager();
+  // Copy so that we can use operator[] to get the children.
+  IncludeStructure Includes = PatchedAST->getIncludeStructure();
+  auto MainFE = FM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(MainFE);
+  auto MainID = Includes.getID(*MainFE);
+  auto AuxFE = FM.getFile(testPath("sub/aux.h"));
+  ASSERT_TRUE(AuxFE);
+  auto AuxID = Includes.getID(*AuxFE);
+  EXPECT_THAT(Includes.IncludeChildren[*MainID], Contains(AuxID));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -551,18 +552,20 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto  = ExpectedAST.getSourceManager().getFileManager();
+  // Copy so that we can getOrCreateID().
+  IncludeStructure Includes = ExpectedAST.getIncludeStructure();
+  auto MainFE = FM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(MainFE);
+  auto MainID = Includes.getOrCreateID(*MainFE);
+  auto  = PatchedAST->getSourceManager().getFileManager();
+  IncludeStructure PatchedIncludes = PatchedAST->getIncludeStructure();
+  auto PatchedMainFE = PatchedFM.getFile(testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedMainFE);
+  auto PatchedMainID = PatchedIncludes.getOrCreateID(*PatchedMainFE);
+  EXPECT_EQ(Includes.includeDepth(MainID),
+PatchedIncludes.includeDepth(PatchedMainID));
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -29,8 +30,10 @@
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedElementsAreArray;
 
 class HeadersTest : public 

[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(Result)].empty())
+RealPathNames[static_cast(Result)] = RealPathName.str();

extract `std::string  = RealPathNames[...]`?



Comment at: clang-tools-extra/clangd/Headers.cpp:185
+  llvm::DenseMap Result;
+  if (static_cast(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  = RealPathNames[Child];
   // Can't include files if we don't have their real path.
+  if (!RealPathNames[static_cast(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 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> 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 SM) {
+  auto Flatten = [](const llvm::DenseMap
+,

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  = PatchedAST->getSourceManager().getFileManager();
auto  = 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: 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 375151.
kbobyrev added a comment.

Rebase on top of main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -47,6 +47,7 @@
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAreArray;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast(arg))
@@ -507,18 +508,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(ExpectedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure()),
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+  ExpectedAST.getIncludeStructure()));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -551,18 +565,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  // Ensure file proximity signals are correct.
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(PatchedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure())
+  .size(),
+  1u);
+  EXPECT_TRUE(
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*PatchedEntryID),
+  ExpectedAST.getIncludeStructure())
+  .empty());
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -31,6 +32,7 @@
 using ::testing::ElementsAre;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
+using 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 375149.
kbobyrev marked 9 inline comments as done.
kbobyrev added a comment.

Resolve all review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -47,6 +47,7 @@
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAreArray;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast(arg))
@@ -507,18 +508,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(ExpectedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure()),
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+  ExpectedAST.getIncludeStructure()));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -551,18 +565,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  // Ensure file proximity signals are correct.
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(PatchedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure())
+  .size(),
+  1u);
+  EXPECT_TRUE(
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*PatchedEntryID),
+  ExpectedAST.getIncludeStructure())
+  .empty());
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -31,6 +32,7 @@
 using ::testing::ElementsAre;
 using ::testing::Not;
 using 

[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 , @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 ProxSources;
-  for (auto  : Includes.includeDepth(
-   SM.getFileEntryForID(SM.getMainFileID())->getName())) {
-auto  = 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 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 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 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 374863.
kbobyrev added a comment.

Clean tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -47,6 +47,7 @@
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAreArray;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast(arg))
@@ -507,18 +508,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(PatchedEntryID)) << PatchedEntryID.takeError();
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(ExpectedEntryID)) << ExpectedEntryID.takeError();
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure()),
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+  ExpectedAST.getIncludeStructure()));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -551,18 +565,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  // Ensure file proximity signals are correct.
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(bool(PatchedEntryID)) << PatchedEntryID.takeError();
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure())
+  .size(),
+  1u);
+  EXPECT_TRUE(
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*PatchedEntryID),
+  ExpectedAST.getIncludeStructure())
+  .empty());
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -31,6 +32,7 @@
 using 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 374853.
kbobyrev added a comment.
Herald added a subscriber: mgrang.

Fix the last failing test. All tests are green now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -507,18 +507,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(static_cast(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(static_cast(ExpectedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure()),
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+  ExpectedAST.getIncludeStructure()));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -551,18 +564,9 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
-std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
-llvm::sort(Res);
-return Res;
-  };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  EXPECT_TRUE(PatchedAST->getIncludeStructure().IncludeChildren.empty());
+  EXPECT_TRUE(ExpectedAST.getIncludeStructure().IncludeChildren.empty());
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -31,6 +32,7 @@
 using ::testing::ElementsAre;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedElementsAreArray;
 
 class HeadersTest : public ::testing::Test {
 public:
@@ -64,8 +66,17 @@
   }
 
 protected:
+  IncludeStructure::HeaderID getID(StringRef Filename) {
+auto Includes = collectIncludes();
+auto Entry = Clang->getSourceManager().getFileManager().getFile(Filename);
+EXPECT_TRUE(Entry);
+llvm::Expected EntryID = Includes.getID(*Entry);
+EXPECT_TRUE(static_cast(EntryID));
+return *EntryID;
+  }
+
   IncludeStructure collectIncludes() {
-auto Clang = setupClang();
+Clang = setupClang();
 PreprocessOnlyAction Action;
 EXPECT_TRUE(
 Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
@@ -81,7 +92,7 @@
   // inserted.
   std::string calculate(PathRef Original, PathRef Preferred = "",
 const std::vector  = {}) {
-auto Clang = setupClang();
+Clang = 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

I resolved the comments we discussed offline, there's a weird SIGSEGV (out of 
boundary error) somewhere in the tests but it looks fine otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 374841.
kbobyrev added a comment.

Update non-working test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -505,18 +505,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(static_cast(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(static_cast(ExpectedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure()),
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+  ExpectedAST.getIncludeStructure()));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -549,18 +562,31 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(static_cast(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID = PatchedAST->getIncludeStructure().getID(*PatchedEntry);
+  ASSERT_TRUE(static_cast(ExpectedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure()),
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+  ExpectedAST.getIncludeStructure()));
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -31,6 +32,7 @@
 using ::testing::ElementsAre;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedElementsAreArray;
 
 class HeadersTest : 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 374807.
kbobyrev added a comment.

s/ToVector/Flatten/g


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -505,18 +505,33 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID =
+  PatchedAST->getIncludeStructure().getFile(*PatchedEntry);
+  ASSERT_TRUE(static_cast(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID =
+  PatchedAST->getIncludeStructure().getFile(*PatchedEntry);
+  ASSERT_TRUE(static_cast(ExpectedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure()),
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+  ExpectedAST.getIncludeStructure()));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -549,18 +564,33 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto Flatten = [](const llvm::DenseMap
+,
+const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID =
+  PatchedAST->getIncludeStructure().getFile(*PatchedEntry);
+  ASSERT_TRUE(static_cast(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID =
+  PatchedAST->getIncludeStructure().getFile(*PatchedEntry);
+  ASSERT_TRUE(static_cast(ExpectedEntryID));
+  EXPECT_EQ(
+  Flatten(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+  PatchedAST->getIncludeStructure()),
+  Flatten(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+  ExpectedAST.getIncludeStructure()));
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -31,6 +32,7 @@
 using ::testing::ElementsAre;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
+using 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 374806.
kbobyrev marked 9 inline comments as done.
kbobyrev added a comment.

Resolve review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -505,18 +505,33 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto ToVector = [](const llvm::DenseMap
+ ,
+ const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID =
+  PatchedAST->getIncludeStructure().getFile(*PatchedEntry);
+  ASSERT_TRUE(static_cast(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID =
+  PatchedAST->getIncludeStructure().getFile(*PatchedEntry);
+  ASSERT_TRUE(static_cast(ExpectedEntryID));
+  EXPECT_EQ(
+  ToVector(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+   PatchedAST->getIncludeStructure()),
+  ToVector(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+   ExpectedAST.getIncludeStructure()));
 }
 
 TEST(ParsedASTTest, PatchesDeletedIncludes) {
@@ -549,18 +564,33 @@
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
   testing::Pointwise(
   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
-  auto StringMapToVector = [](const llvm::StringMap SM) {
+  auto ToVector = [](const llvm::DenseMap
+ ,
+ const IncludeStructure ) {
 std::vector> Res;
-for (const auto  : SM)
-  Res.push_back({E.first().str(), E.second});
+for (const auto  : IncludeDepth)
+  Res.emplace_back(Structure.getRealPath(E.first).str(), E.second);
 llvm::sort(Res);
 return Res;
   };
   // Ensure file proximity signals are correct.
-  EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
-testPath("foo.cpp"))),
-StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
-testPath("foo.cpp";
+  auto PatchedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(PatchedEntry);
+  auto PatchedEntryID =
+  PatchedAST->getIncludeStructure().getFile(*PatchedEntry);
+  ASSERT_TRUE(static_cast(PatchedEntryID));
+  auto ExpectedEntry = PatchedAST->getSourceManager().getFileManager().getFile(
+  testPath("foo.cpp"));
+  ASSERT_TRUE(ExpectedEntry);
+  auto ExpectedEntryID =
+  PatchedAST->getIncludeStructure().getFile(*PatchedEntry);
+  ASSERT_TRUE(static_cast(ExpectedEntryID));
+  EXPECT_EQ(
+  ToVector(PatchedAST->getIncludeStructure().includeDepth(*PatchedEntryID),
+   PatchedAST->getIncludeStructure()),
+  ToVector(ExpectedAST.getIncludeStructure().includeDepth(*ExpectedEntryID),
+   ExpectedAST.getIncludeStructure()));
 }
 
 // Returns Code guarded by #ifndef guards
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -17,6 +17,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
@@ -31,6 +32,7 @@
 using ::testing::ElementsAre;
 using ::testing::Not;
 using 

[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 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 ")



Comment at: clang-tools-extra/clangd/Headers.h:124
+
+  llvm::ArrayRef 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 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 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 includeDepth(llvm::StringRef Root) const;
+  llvm::StringMap 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 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>;
+

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
Herald added subscribers: usaxena95, arphaman.
kbobyrev requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Preparation for D108194 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110386

Files:
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h

Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -62,7 +62,7 @@
 llvm::raw_ostream <<(llvm::raw_ostream &, const Inclusion &);
 bool operator==(const Inclusion , const Inclusion );
 
-// Contains information about one file in the build grpah and its direct
+// Contains information about one file in the build graph and its direct
 // dependencies. Doesn't own the strings it references (IncludeGraph is
 // self-contained).
 struct IncludeGraphNode {
@@ -112,6 +112,22 @@
 // in any non-preamble inclusions.
 class IncludeStructure {
 public:
+  using File = unsigned;
+
+  llvm::Optional getFile(StringRef Name) const {
+auto It = NameToIndex.find(Name);
+if (It == NameToIndex.end() || Name.empty())
+  return llvm::None;
+return It->second;
+  }
+
+  llvm::ArrayRef getIncludedFiles(File Index) const {
+auto It = IncludeChildren.find(Index);
+if (It == IncludeChildren.end())
+  return {};
+return It->second;
+  }
+
   std::vector MainFileIncludes;
 
   // Return all transitively reachable files.
@@ -122,24 +138,27 @@
   // Root --> 0, #included file --> 1, etc.
   // Root is clang's name for a file, which may not be absolute.
   // Usually it should be SM.getFileEntryForID(SM.getMainFileID())->getName().
-  llvm::StringMap includeDepth(llvm::StringRef Root) const;
+  llvm::StringMap includeDepth(StringRef Name) const;
 
   // This updates IncludeDepth(), but not MainFileIncludes.
   void recordInclude(llvm::StringRef IncludingName,
  llvm::StringRef IncludedName,
  llvm::StringRef IncludedRealName);
 
+  using AbstractIncludeGraph = llvm::DenseMap>;
+
 private:
+  File getOrCreatefileIndex(llvm::StringRef Name);
+
   // Identifying files in a way that persists from preamble build to subsequent
   // builds is surprisingly hard. FileID is unavailable in InclusionDirective(),
   // and RealPathName and UniqueID are not preserved in the preamble.
   // We use the FileEntry::Name, which is stable, interned into a "file index".
   // The paths we want to expose are the RealPathName, so store those too.
   std::vector RealPathNames; // In file index order.
-  unsigned fileIndex(llvm::StringRef Name);
-  llvm::StringMap NameToIndex; // Values are file indexes.
+  llvm::StringMap NameToIndex;  // Values are file indexes.
   // Maps a file's index to that of the files it includes.
-  llvm::DenseMap> IncludeChildren;
+  AbstractIncludeGraph IncludeChildren;
 };
 
 /// Returns a PPCallback that visits all inclusions in the main file.
Index: clang-tools-extra/clangd/Headers.cpp
===
--- clang-tools-extra/clangd/Headers.cpp
+++ clang-tools-extra/clangd/Headers.cpp
@@ -157,31 +157,33 @@
 void IncludeStructure::recordInclude(llvm::StringRef IncludingName,
  llvm::StringRef IncludedName,
  llvm::StringRef IncludedRealName) {
-  auto Child = fileIndex(IncludedName);
+  auto Child = getOrCreatefileIndex(IncludedName);
   if (!IncludedRealName.empty() && RealPathNames[Child].empty())
 RealPathNames[Child] = std::string(IncludedRealName);
-  auto Parent = fileIndex(IncludingName);
+  auto Parent = getOrCreatefileIndex(IncludingName);
   IncludeChildren[Parent].push_back(Child);
 }
 
-unsigned IncludeStructure::fileIndex(llvm::StringRef Name) {
+IncludeStructure::File
+IncludeStructure::getOrCreatefileIndex(llvm::StringRef Name) {
   auto R = NameToIndex.try_emplace(Name, RealPathNames.size());
   if (R.second)
 RealPathNames.emplace_back();
   return R.first->getValue();
 }
 
-llvm::StringMap
-IncludeStructure::includeDepth(llvm::StringRef Root) const {
+llvm::StringMap IncludeStructure::includeDepth(StringRef Name) const {
   // Include depth 0 is the main file only.
   llvm::StringMap Result;
-  Result[Root] = 0;
+  Result[Name] = 0;
   std::vector CurrentLevel;
-  llvm::DenseSet Seen;
-  auto It = NameToIndex.find(Root);
-  if (It != NameToIndex.end()) {
-CurrentLevel.push_back(It->second);
-Seen.insert(It->second);
+  llvm::DenseSet Seen;
+  auto Root = getFile(Name);
+  if (Root) {
+CurrentLevel.push_back(*Root);
+Seen.insert(*Root);
+  } else {
+elog("Can not find the root ({0}) for includeDepth.", Name);
   }
 
   // Each round of BFS traversal finds the next depth level.