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