[PATCH] D136539: [Lex] Bring back the magic number 50 in updateConsecutiveMacroArgTokens.

[Haojian Wu via Phabricator via cfe-commits]

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