[PATCH] D66751: [clangd] Add targetDecl(), which determines what declaration an AST node refers to.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I agree with Ilya's concerns on SourceLocations, even though most of the time user will have the source locations available in the dyntyped node, they might need some traversals to fetch the exact range especially in cases of nested name specifiers.
It would be nice to provide a referencing range in addition to the decls.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:79
+  static const Decl *getTemplatePattern(const Decl *D) {
+    if (const CXXRecordDecl *SD = dyn_cast<CXXRecordDecl>(D)) {
+      return SD->getTemplateInstantiationPattern();
----------------
nit: s/SD/CRD/


================
Comment at: clang-tools-extra/clangd/FindTarget.h:97
+class DeclRelationSet {
+  using Set = std::bitset<static_cast<unsigned>(DeclRelation::PureLexical) + 1>;
+  Set S;
----------------
Could you rather put an end marker into the `DeclRelation` and make use of it in here? As people might forget to update this one if they add anything into DeclRelation.

(It seems rare for this enum to change though, so feel free to ignore if you believe it will stay the same).


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:49
+    EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty()) << Code;
+    llvm::Annotations::Range R = A.llvm::Annotations::range();
+    SelectionTree Selection(AST.getASTContext(), AST.getTokens(), R.Begin,
----------------
can't you just call `A.range()` ? :D
I hope it wasn't clangd that somehow code-completed this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66751