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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
