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