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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D87891#2291838 <https://reviews.llvm.org/D87891#2291838>, @ArcsinX wrote:

> Thanks for your reply, I will rethink this patch.

FWIW I think if we drop most of the math in favor of using SourceManager's line translation, this doesn't add much complexity and is probably a bit more direct and efficient.



================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87891