[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:589
     // but not things like disabled preprocessor sections.
     if (!(tokenSpelledAt(Tok.location(), TB) || TB.expansionStartingAt(&Tok)))
       return false;
----------------
here


================
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)
----------------
ArcsinX wrote:
> 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).
I failed to create a test for this.
I can create a test for identifiers in a macro body  or under `#ifdef`, but such a test passes without ` if (Tok.location() == Word.Location)` because of this check:
```
    if (!(tokenSpelledAt(Tok.location(), TB) || TB.expansionStartingAt(&Tok)))
      return false;
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87891