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



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

Reply via email to