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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
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();
----------------
kadircet wrote:
> nit: s/SD/CRD/
Done.
(This was copy/pasted from libIndex, I wonder what SD stood for. StructDecl?)


================
Comment at: clang-tools-extra/clangd/FindTarget.h:86
+  /// This declaration is an alias that was referred to.
+  Alias,
+  /// This is the underlying declaration for an alias, decltype etc.
----------------
ilya-biryukov wrote:
> Could you provide examples for `Alias` and `Underlying` in the comments?
> One with a template and one with a namespace alias
Done. Added the code example as a section above the values, and the results with the comment on each value - hope this isn't too confusing.


================
Comment at: clang-tools-extra/clangd/FindTarget.h:92
+  /// (e.g. a delegating constructor has a pure-lexical reference to the class)
+  PureLexical,
+};
----------------
ilya-biryukov wrote:
> Could you also provide an example here?
> It this a delegating constructor in the constructor-init-list?
It was, but it was misguided - the delegating constructor calls have a TypeLoc which references the type being delegated to, so there's not much point handling them I think.

(There would be if we could get the constructor, but we can't: I guess in general it may not be resolved)

This was the only user of PureLexical, so I just removed it.



================
Comment at: clang-tools-extra/clangd/FindTarget.h:97
+class DeclRelationSet {
+  using Set = std::bitset<static_cast<unsigned>(DeclRelation::PureLexical) + 1>;
+  Set S;
----------------
kadircet wrote:
> 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).
Hmm, I'm reluctant to add a "last" member to a strongly-typed enum, and force switch to handle it etc.

I could add it next to the enum, but then it's ugly and still easy enough to miss.

Ultimately, I don't think it's likely this will be missed: `bitset::set()` checks its argument and throws out_of_range (i.e. aborts) if we try to set an invalid bit. So any test that exercised a new value would crash in all build configs.


================
Comment at: clang-tools-extra/clangd/FindTarget.h:103
+  DeclRelationSet() = default;
+  DeclRelationSet(DeclRelation R) { S.set(static_cast<unsigned long long>(R)); }
+
----------------
ilya-biryukov wrote:
> Why not `unsigned`? What are the interesting corner cases?
Oops, this dates back to when I was prematurely-templatizing this class. Reverted.


================
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,
----------------
kadircet wrote:
> can't you just call `A.range()` ? :D
> I hope it wasn't clangd that somehow code-completed this.
A.range() is an LSP Location (line/col pairs), offsets seem slightly more convenient here.

This is ugly though, using Annotations with SourceLocations seems unneccesarily clunky. Ideas?

(this reminds me, these test helpers need comments, added some)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66751