[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:152
+  // Attribute relations to both their subject's and object's locations.
+  // See https://github.com/clangd/clangd/issues/510 for discussion of why.
   if (Index.Relations) {
----------------
instead of (or in addition to) providing the link, can we give a short summary? e.g.

```
Subject and/or Object files might be part of multiple TUs. In such cases there will be a race
and last TU to write the shard will win and all the other relations will be lost. We are storing
relations in both places, as we do for symbols, to reduce such races. Note that they might still
happen if same translation unit produces different relations under different configurations
but that's something clangd doesn't handle in general. 
```


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:350
       AllRelations.push_back(R);
+    // Sort relations and remove duplicates that could arise due to
+    // relations being stored in both the shards containing their
----------------
can you move this outside of the for loop, so that we do it only once.


================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232
 
+TEST_F(BackgroundIndexTest, RelationsMultiFile) {
+  MockFS FS;
----------------
do we still need this test?


================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:396
   SymbolID B = findSymbol(*ShardSource->Symbols, "B_CC").ID;
-  EXPECT_THAT(*ShardHeader->Relations,
+  EXPECT_THAT(*ShardSource->Relations,
+              UnorderedElementsAre(Relation{A, RelationKind::BaseOf, B}));
----------------
s/ShardSource/ShardHeader


================
Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:571
     B.insert(Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID});
-    // Dangling relation should be dropped.
-    B.insert(Relation{symbol("3").ID, RelationKind::BaseOf, Sym1.ID});
+    // Should be stored in a.h even though it's dangling
+    B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID});
----------------
maybe mention that `Sym1` belongs to `a.h` in the comment. `stored in a.h (where Sym1 is stored) even tho the reference is dangling as Sym3 is unknown `


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87256