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

[Sam McCall via Phabricator via cfe-commits]

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