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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
