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