[PATCH] D144054: [Lex] Fix a crash in updateConsecutiveMacroArgTokens.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1023
     Partition = All.take_while([&](const Token &T) {
-      return T.getLocation() >= BeginLoc && T.getLocation() < Limit &&
-             NearLast(T.getLocation());
+      // NOTE: the Limit is included! In general, all token locations
+      // should be within [BeginLoc, Limit) range. However, the clang
----------------
shafik wrote:
> You had refactored the previous version, was this the way it was handled before? Do we need to add more test to ensure that we don't have any other unintended issues?
>  was this the way it was handled before?
Yeah, the previous implementation literally checked whether every token is from the same file id by calling `getFileID`.

> Do we need to add more test to ensure that we don't have any other unintended issues?

I'd like to add more, but it is hard to foresee all invalid cases (clang has different error-recovery strategies that might have different side effects). 


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1031
+      //
+      // See https://github.com/llvm/llvm-project/issues/60722.
+      return T.getLocation() >= BeginLoc && T.getLocation() <= Limit
----------------
shafik wrote:
> I don't believe we include links to issues in comments but you should definitely add the information to the commit message and when folks look at the commit they can get the that context there.
I'm happy to remove it. But is there any guideline discouraging this practise? I have already seen this pattern in LLVM project. I think this is based on the author's judgement (IMO, it seems better to keep it for this case).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144054