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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This is a subtle change that needs careful review, and honestly should have a test.

I realize there's a breakage that needs to be fixed with some urgency and you believe you're just restoring the old behavior, however in that case the right course of action is to revert the patch.
As it turns out I think this introduces a bug when offset space is nearly full.



================
Comment at: clang/lib/Lex/TokenLexer.cpp:1009
+      if (T.getLocation().isFileID()) {
+        unsigned distance =
+            T.getLocation().getRawEncoding() - LastLoc.getRawEncoding();
----------------
distance -> Distance


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1010
+        unsigned distance =
+            T.getLocation().getRawEncoding() - LastLoc.getRawEncoding();
+        LastLoc = T.getLocation();
----------------
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



================
Comment at: clang/lib/Lex/TokenLexer.cpp:1010
+        unsigned distance =
+            T.getLocation().getRawEncoding() - LastLoc.getRawEncoding();
+        LastLoc = T.getLocation();
----------------
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())


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1022
         SM.getComposedLoc(BeginFID, SM.getFileIDSize(BeginFID));
+    SourceLocation LastLoc = BeginLoc;
     Partition = All.take_while([&](const Token &T) {
----------------
having worked quite hard to make this code readable, copy/pasting the same (buggy) algorithm in both places seems like an unfortunate choice.

I'd suggest:
```
auto NearLast = [&, Last{BeginLoc}](SourceLocation Loc) mutable {
  constexpr int Limit = 50;
  SourceLocation IntTy  RelOffs;
  if (!SM.isInSameSLocAddrSpace(Last, Loc, &RelOffs) || RelOffs < 0 || RelOffs > Limit)
    return false;
  Last = Loc;
  return true;
};

...
Partition = All.take_while([&]{
  return T.getLocation() >= BeginLoc && T.getLocation() < Limit && NearLast(T.getLocation());
});
```

Here NearLast can be shared between both branches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136539