[PATCH] D135132: [SourceManager] Improve getFileIDLocal.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice!



================
Comment at: clang/lib/Basic/SourceManager.cpp:796
+  // larger than SLocOffset.
+  const SrcMgr::SLocEntry *I = LocalSLocEntryTable.end();
+  // LessIndex - This is the lower bound of the range that we're searching.
----------------
hmm, while here, why not just get rid of `I` and use `GreaterIndex` throughout?

Of its uses: line 806 and 815 would be clearer, 814 would be less clear, 796 and 813 are the same, and 827 goes away entirely.

(This is a little thing to nitpick, but I always hate trying to read this function)


================
Comment at: clang/lib/Basic/SourceManager.cpp:809
 
   // Find the FileID that contains this.  "I" is an iterator that points to a
   // FileID whose offset is known to be larger than SLocOffset.
----------------
this comment duplicates your new one above


================
Comment at: clang/lib/Basic/SourceManager.cpp:897
   // actually a lower index!
   unsigned GreaterIndex = I;
   unsigned LessIndex = LoadedSLocEntryTable.size();
----------------
is the same optimization available here?
I think getFileIDLoaded() is important for module builds and when we use PCH.
Though admittedly I don't think this is relevant to the lexer path we've been looking at: those locations should always be local.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135132