hokein added a comment. > This is a subtle change that needs careful review, and honestly should have a > test.
I agree, and thanks for the nice review comments! I'd like to add a unittest, but I don't see a straight way (I manually test it by comparing the number of allocated SLocEntries and the used SourceLocation address space in `clang --print-stats` with/out this patch), some options: 1. construct a large TU that will make clang fails to compile without the 50 trick patch; 2. create a small test, and verify the the source location is split when the two consecutive tokens distance > 50, and verify the number of in `clang print-stats`; 1 feels better to reflect the real behavior, the only concern is that this is a stress test, running it is expensive (we also have a similar `SourceLocationsOverlow.c`, I guess it is ok to add it). ================ Comment at: clang/lib/Lex/TokenLexer.cpp:1010 + unsigned distance = + T.getLocation().getRawEncoding() - LastLoc.getRawEncoding(); + LastLoc = T.getLocation(); ---------------- sammccall wrote: > 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()) > missing the same-sloc-address-space check oops, good spot. Indeed this is another behavior change in the previous-rewriting patch :( -- the original behavior was that if two consecutive tokens are not in the same address space, it split to two expansion SLocEntries. Fixed. 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
