[PATCH] D70357: [clangd] Untangle Hover from XRefs, move into own file.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 18 12:57:54 PST 2019
sammccall marked 4 inline comments as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:374
+ } else {
+ auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
+ if (!Offset) {
----------------
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)
================
Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:13
+#include "TestTU.h"
+
+#include "clang/Basic/SourceManager.h"
----------------
kadircet wrote:
> unintended newline ?
intended (subproject vs clang) but dosen't seem to match local style. removed
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70357/new/
https://reviews.llvm.org/D70357
More information about the cfe-commits
mailing list