[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:374
+  } else {
+    auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
+    if (!Offset) {
----------------
sammccall wrote:
> kadircet wrote:
> > kadircet wrote:
> > > nit: `SM.getFileOffset(SourceLocationBeg)` ?
> > why not just expose getDeclAtPosition in `AST.h` ?
> > nit: SM.getFileOffset(SourceLocationBeg) ?
> I was deliberately trying to avoid using the "rewind token" logic where it's not needed. We've had bugs with it before, as well as with selectiontree, and composing them unneccesarily is harder to debug.
> 
> > why not just expose getDeclAtPosition in AST.h ?
> I don't think it's better than the expanded form - it's just plugging a couple of functions together, and the choices it makes (flattening SourceLocation into an offset, the DeclRelationSet, only looking at the common ancestor and nothing higher up) aren't obvious.
> 
> It also privileges decls over other types of things (e.g. the followup patch looks for Exprs in the selection tree to show their values, and that couldn't happen if it was hidden behind getDeclAtPosition)
> I was deliberately trying to avoid using the "rewind token" logic where it's not needed. We've had bugs with it before, as well as with selectiontree, and composing them unneccesarily is harder to debug.

Sounds good, I was worried about performing this conversion twice, but I suppose it should be OK for hover's performance.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:380
+    SelectionTree Selection(AST.getASTContext(), AST.getTokens(), *Offset);
+    std::vector<const Decl *> Result;
+    if (const SelectionTree::Node *N = Selection.commonAncestor()) {
----------------
nit: Unused


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70357