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

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

In D87891#2291760 <https://reviews.llvm.org/D87891#2291760>, @kadircet wrote:

> The biggest problem I see is, this is not changing the fact that we are still traversing the whole file:
>
> - You do one traversal over the whole file to find `FileLines`
> - Then possibly two more to find `OffsetMin` and `OffsetMax`

Seems you are right, but we do not compare strings during traversal to find `FileLines`.

> You can get rid of the first one by just using `getLocForEndOfFile` if `positionToOffset` fails.
> For the latter, you can use `SourceManager::translateLineCol` instead, it uses a cache and cheaper than the linear scan performed by `positionToOffset` in case of multiple calls. The cache is already populated once we issue the first `Cost` call, through `SM.getSpellingLineNumber`
>
> I was reluctant overall as the wins aren't clear. We are still going to traverse the whole file at least once, and readability is hindered but I suppose with the above mentioned two trics, runtime shouldn't be regressed.
> Also it is nice to get rid of 2 log calls for each token. Again all of these feels like some micro optimizations that aren't justified.

Thanks for your reply, I will rethink this patch.



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:568
     if (Line < WordLine)
-      return 2 + llvm::Log2_64(WordLine - Line);
+      return 2 + WordLine - Line;
     return 0;
----------------
sammccall wrote:
> this has changed the relative ordering, if we're dropping the log then +1 should now become multiplication by two.
> (Or was this intended?)
Yes, you are right, my fault here.

But should we penalize backwards so hard?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87891