[PATCH] D134942: [Lex] Simplify and cleanup the updateConsecutiveMacroArgTokens implementation.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1038
+  for (Token& T : Partition) {
+    SourceLocation::IntTy RelativeOffset = T.getLocation().getRawEncoding() -
+       BeginLoc.getRawEncoding();
----------------
hokein wrote:
> sammccall wrote:
> > hmm, actually maybe just before this line would be the best place to assert that T and BeginLoc are in the same FileID, as it justifies the subtraction
> I agree that this is the best place to put the check-file-id assertion.
> 
> Simply adding `assert(getFileID(BeginLoc) == getFileID(T.getLocation()))` can work, but it creates N-1 unnecessary getFileID calls on BeginLoc for assert build, it is better to avoid it. The only way I can think of is 
> 
> ```
> #ifndef NDEBUG
> FileID BeginFID = SM.getFileID(BeginLoc);
> assert(BeginFID == getFileID(T.getLocation()));
> #endif
> ```
I assume you mean with the #if once above, but the assert outside the #if and inside the loop?
That LGTM

There's also #ifdef EXPENSIVE_CHECKS, maybe not needed here though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134942