[PATCH] D83508: [clangd][Hover] Don't use Decl if it is not related with tokens under cursor.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D83508#2155335 <https://reviews.llvm.org/D83508#2155335>, @ArcsinX wrote:

> In D83508#2155322 <https://reviews.llvm.org/D83508#2155322>, @sammccall wrote:
>
> > In D83508#2155174 <https://reviews.llvm.org/D83508#2155174>, @ArcsinX wrote:
> >
> > > In D83508#2143625 <https://reviews.llvm.org/D83508#2143625>, @sammccall wrote:
> > >
> > > > Your test cases show two problems with this strategy:
> > > >
> > > > - we ignore comments and semicolons, but not preprocessor directives (or disabled preprocessor regions). I think we can fix this by asking TokenBuffer if a spelled token is part of a region that maps to no (PP-)expanded tokens.
> > >
> > >
> > > I have tried this locally. seems it breaks SelectionTest.IncludedFile test.
> >
> >
> > Yeah that makes sense, I guess it just says nothing is selected in that case?
>
>
> Yes, and test crashes at `T.commonAncestor()` (which is `nullptr`) dereference. So can we also add `ASSERT_TRUE(T.commonAncestor());` into several tests?


Right, that particular case should become EXPECT_EQ(..., nullptr).

Technically the other tests should probably have an ASSERT, because otherwise if the code is broken and returns nullptr we have UB and the test could spuriously pass.
In practice I've never seen a test spuriously pass this way, let alone on all buildbots. And these kinds of assumptions are hard to keep out of the tests. So I'm not particularly concerned about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83508