sammccall added a comment.
This is a subtle change that needs careful review, and honestly should have a
test.
I realize there's a breakage that needs to be fixed with some urgency and you
believe you're just restoring the old behavior, however in that case the right
course of action is to revert the patch.
As it turns out I think this introduces a bug when offset space is nearly full.
================
Comment at: clang/lib/Lex/TokenLexer.cpp:1009
+ if (T.getLocation().isFileID()) {
+ unsigned distance =
+ T.getLocation().getRawEncoding() - LastLoc.getRawEncoding();
----------------
distance -> Distance
================
Comment at: clang/lib/Lex/TokenLexer.cpp:1010
+ unsigned distance =
+ T.getLocation().getRawEncoding() - LastLoc.getRawEncoding();
+ LastLoc = T.getLocation();
----------------
This seems to be missing the same-sloc-address-space check: it can create a
single file ID spanning a local and loaded sloc entry, which will be corrupted
by saving+loading
================
Comment at: clang/lib/Lex/TokenLexer.cpp:1010
+ unsigned distance =
+ T.getLocation().getRawEncoding() - LastLoc.getRawEncoding();
+ LastLoc = T.getLocation();
----------------
sammccall wrote:
> This seems to be missing the same-sloc-address-space check: it can create a
> single file ID spanning a local and loaded sloc entry, which will be
> corrupted by saving+loading
>
(getOffset() would be much clearly correct than getRawEncoding())
================
Comment at: clang/lib/Lex/TokenLexer.cpp:1022
SM.getComposedLoc(BeginFID, SM.getFileIDSize(BeginFID));
+ SourceLocation LastLoc = BeginLoc;
Partition = All.take_while([&](const Token &T) {
----------------
having worked quite hard to make this code readable, copy/pasting the same
(buggy) algorithm in both places seems like an unfortunate choice.
I'd suggest:
```
auto NearLast = [&, Last{BeginLoc}](SourceLocation Loc) mutable {
constexpr int Limit = 50;
SourceLocation IntTy RelOffs;
if (!SM.isInSameSLocAddrSpace(Last, Loc, &RelOffs) || RelOffs < 0 || RelOffs
> Limit)
return false;
Last = Loc;
return true;
};
...
Partition = All.take_while([&]{
return T.getLocation() >= BeginLoc && T.getLocation() < Limit &&
NearLast(T.getLocation());
});
```
Here NearLast can be shared between both branches.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136539/new/
https://reviews.llvm.org/D136539
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
