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

sammccall added a comment. Herald added a subscriber: kbobyrev. In D83508#2157954 , @ArcsinX wrote: > In D83508#2157859 , @sammccall wrote: > > > Tried this out in D84012 /D84009 >

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

ArcsinX added a comment. In D83508#2157859 , @sammccall wrote: > Tried this out in D84012 /D84009 > . Works pretty well, and I think the API is > a useful and natural addition to

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

sammccall added a comment. Tried this out in D84012 /D84009 . Works pretty well, and I think the API is a useful and natural addition to TokenBuffer. Maybe this is too much complexity though? (Mostly here i'm worrying about the

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:227 continue; + // Ignore tokens in disabled preprocessor sections. + if (Buf.expandedTokens(SM.getMacroArgExpandedLocation(T->location())) ArcsinX wrote: >

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

ArcsinX added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:227 continue; + // Ignore tokens in disabled preprocessor sections. + if (Buf.expandedTokens(SM.getMacroArgExpandedLocation(T->location())) sammccall wrote: > I

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

sammccall added a comment. (Sorry, I know that this is a lot of goalpost-shifting: I think this is now being solved at the right layer and with the right behavior, and it's just a question of finding a clear+fast implementation) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:227 continue; + // Ignore tokens in disabled preprocessor sections. + if (Buf.expandedTokens(SM.getMacroArgExpandedLocation(T->location())) I think this is

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

ArcsinX updated this revision to Diff 278440. ArcsinX added a comment. Prevent tokens selection in disabled preprocessor sections. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83508/new/ https://reviews.llvm.org/D83508 Files:

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

kadircet added a comment. In D83508#2155322 , @sammccall wrote: > Yeah that makes sense, I guess it just says nothing is selected in that case? > Selecting the `while` is probably marginally better for that exact case, but > selecting nothing seems fine

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

sammccall added a comment. In D83508#2155335 , @ArcsinX wrote: > In D83508#2155322 , @sammccall wrote: > > > In D83508#2155174 , @ArcsinX wrote: > > > > > In D83508#2143625

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

ArcsinX added a comment. In D83508#2155322 , @sammccall wrote: > In D83508#2155174 , @ArcsinX wrote: > > > In D83508#2143625 , @sammccall > > wrote: > > > > > Your test

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

sammccall added a comment. In D83508#2155174 , @ArcsinX wrote: > In D83508#2143625 , @sammccall wrote: > > > Your test cases show two problems with this strategy: > > > > - we ignore comments and semicolons, but

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

ArcsinX added a comment. In 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

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

sammccall added a comment. Yeah SelectionTree is the right place to solve this but it's a harder problem there. In general we *don't* only want to trigger on the single token returned by getLocation(). The strategy SelectionTree uses is to attribute tokens to an outer node unless they're

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

ArcsinX added a comment. @kadircet Thanks for your reply. Think that you are right, because the same problem appears in GTD (with the same test cases). And as for GTD the similar workaround also does not break any regression tests except override/final. Repository: rG LLVM Github Monorepo

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

kadircet added a comment. thanks for doing this! but i believe these look like bugs that should be addressed in other components, rather than worked around in hover. the first example is likely a bug in selection tree, file location corresponds to a macro body that's never expanded, which

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

ArcsinX created this revision. Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. This patch fixes redundant hover with information about Decl which is not under cursor. Repository: rG LLVM Github Monorepo