[PATCH] D135132: [SourceManager] Improve getFileIDLocal.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Basic/SourceManager.cpp:797
-      LocalSLocEntryTable[LastFileIDLookup.ID].getOffset() < SLocOffset) {
-    // Neither loc prunes our search.
-    I = LocalSLocEntryTable.end();
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > Consider renaming this `LesserIndex` which matches with `GreaterIndex` better.
> So this comment was wrong?
This comment was true before the patch. I have updated it to match the current code.


================
Comment at: clang/lib/Basic/SourceManager.cpp:797
+  // SLocOffset.
+  unsigned LessIndex = 0;
+  // upper bound of the search range.
----------------
hokein wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > Consider renaming this `LesserIndex` which matches with `GreaterIndex` better.
> > So this comment was wrong?
> This comment was true before the patch. I have updated it to match the current code.
I'm not sure, it doesn't seem to be much better (I'm leaning towards keeping it as-is).


================
Comment at: clang/lib/Basic/SourceManager.cpp:807
   }
 
   // Find the FileID that contains this.  "I" is an iterator that points to a
----------------
sammccall wrote:
> Just checking: we decrement GreaterIndex without a bounds check in the loop and then dereference it in the loop.
> 
> I guess this is safe because:
> - in the case where it's equal to size(), it's nonzero due to a dummy entry at 0 (and unchanged in this patch)
> - in the case where the cache is used, it's also nonzero (guarded)
> 
> Maybe this is worth an assertion?
yes, exactly. added an assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135132