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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D136539#3882316 <https://reviews.llvm.org/D136539#3882316>, @hokein wrote:

>> 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).

I was all ready to say that option 2 is the only practical one, and was surprised that you had an actual testcase.
I'm a bit nervous (it must be slow and eat tons of ram) but if we already have a similar test...



================
Comment at: clang/lib/Lex/TokenLexer.cpp:1010
+        unsigned distance =
+            T.getLocation().getRawEncoding() - LastLoc.getRawEncoding();
+        LastLoc = T.getLocation();
----------------
hokein wrote:
> 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.
> oops, good spot. Indeed this is another behavior change in the previous-rewriting patch :( 

Argh, I'm so sorry, this prompted me to take another look and there is no bug/need for the check at all.

Before we do the NearLast check we've already established that the FileID is the same, which means the locations are necessarily in the same address space.

We can replace the isInSameSLocAddrSpace with a raw offset comparison again...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136539/new/

https://reviews.llvm.org/D136539