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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

LG other than removing the redundant same-sloc-address check again (sorry!)



================
Comment at: clang/lib/Lex/TokenLexer.cpp:1010
+        unsigned distance =
+            T.getLocation().getRawEncoding() - LastLoc.getRawEncoding();
+        LastLoc = T.getLocation();
----------------
hokein wrote:
> sammccall wrote:
> > 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...
> hmm, I think there was some misunderstanding here -- I thought you meant a corner case where Last and T are in the same file, but Last.offset < `CurrentLoadedOffset`, and T.offset > `CurrentLoadedOffset`. For this case, there is a behavior change between both versions. 
> 
> I'm not sure whether this case is possible or it is reasonable to handle it, since it is a running-out-of-sourcelocation-space case (clang's behavior is somewhat inconsistent, in CreateFileID, we emit a too-large diagnostic, in `createExpansionLocImpl`, it is just an assertion), maybe we should not worry about this case, treat it as UB.
> hmm, I think there was some misunderstanding here -- I thought you meant a corner case where Last and T are in the same file, but Last.offset < CurrentLoadedOffset and T.offset > CurrentLoadedOffset

That's not possible, loaded vs local is a property of the FileID. I was missing that we'd established they're in the same FileID before checking deltas.


================
Comment at: clang/test/Lexer/update_consecutive_macro_address_space.c:2
+// RUN: %clang -cc1 -print-stats %s 2>&1 | FileCheck %s
+// CHECK: 6 local SLocEntry's allocated
+//
----------------
it's a shame there's no -cc1 flag for SourceManager::dump() or this could be much more direct...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136539