[PATCH] D87891: [clangd] findNearbyIdentifier(): guaranteed to give up after 2^N lines

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:580
       return false;
-    // No point guessing the same location we started with.
-    if (Tok.location() == Word.Location)
----------------
sammccall wrote:
> ArcsinX wrote:
> > sammccall wrote:
> > > why can't this happen anymore?
> > As I understand, we call `findNearbyIdentifier()` only when the word is not an identifier. And `Tok` is always identifier here.
> > 
> > Btw, even if `findNearbyIdentifier()` could be called for an identifier, I could not get why it's bad to return current position here.
> > 
> > Am I wrong?
> > As I understand, we call findNearbyIdentifier() only when the word is not an identifier
> 
> I'm not sure where you're getting that.
> If you mean the bail-out inside the function itself:
> 
> ```
>   // Don't use heuristics if this is a real identifier.
>   // Unlikely identifiers are OK if they were used as identifiers nearby.
>   if (Word.ExpandedToken)
>     return nullptr;
> ```
> 
> then "real identifier" here means it's an identifier after preprocessing.
> We're still getting here if it's e.g. part of a macro body, or code that's `#ifdef`'d out.
> The spelled token in those cases is an identifier, there's just no corresponding expanded token.
> 
> (I'm surprised we don't have a test covering this though!)
> 
> > Btw, even if findNearbyIdentifier() could be called for an identifier, I could not get why it's bad to return current position here.
> 
> The point of this function is that *this* occurrence of the word can't be resolved, so let's try another one nearby.
> If we just return the same occurrence, then we're certainly not going to be able to resolve it.
Thanks for your clarification. I will revert this change (and will try to add a test as well).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87891