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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
