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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits