[PATCH] D135258: [SourceManager] Improve getFileIDLoaded.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice!

Just a couple of comments where we could take the opportunity to clarify the existing code. Optional.



================
Comment at: clang/lib/Basic/SourceManager.cpp:874
+    int LastID = LastFileIDLookup.ID;
+    if (getLoadedSLocEntryByID(LastID).getOffset() >= SLocOffset)
+      GreaterIndex = (-LastID - 2) + 1;
----------------
This is confusing: if SLocEntry.getOffset() > SLocOffset then it means SLocOffset is *not* part the entry (entry can be pruned), but if they're equal it means it *is* part of the entry (entry should not be pruned).
Then we're pruning the entry regardless.

I think we're getting away with this because we never get here in the `==` case as we would have hit the cache.
But `>` seems clearer than `>=`, assuming my analysis is right.


================
Comment at: clang/lib/Basic/SourceManager.cpp:875
+    if (getLoadedSLocEntryByID(LastID).getOffset() >= SLocOffset)
+      GreaterIndex = (-LastID - 2) + 1;
+    else
----------------
I believe the `+1` here is in order to exclude `LastFileIDLookup.ID`, and again the justification is that we would otherwise have hit the cache.

I think a short comment `// Exclude LastID, else we would have hit the cache` would help here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135258