[PATCH] D89670: [clangd] Store the containing symbol for refs

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, LGTM!



================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:782
+
+    EXPECT_EQ(Ref1->Container, Ref2->Container);
+  };
----------------
can you also assert containers here are non-null (and below)


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:803
+
+  AssertSameContainer("toplevel1", "toplevel2");
+  AssertSameContainer("classscope1", "classscope2");
----------------
nit: i would suggest writing these in the form of:

```
auto Container = [](StringRef RangeName){
  auto Ref = findRefWithRange(RangeName;
  ASSERT_TRUE(Ref);
  ASSERT_FALSE(Ref->Container.isNull());
  return Ref->Container;
};
EXPECT_EQ(Container("sym1"), Container("sym2"));
EXPECT_NE(Container("sym1"), Container("sym2"));
```

then you could update the ones above as:
```
EXPECT_EQ(Container("rangename"), findSymbol(Symbols, "symname"));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89670

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