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