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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1019
+    // sourcelocation-against-bounds comparison.
+    FileID BeginFID = SM.getFileID(BeginLoc);
+    SourceLocation Limit =
----------------
sammccall wrote:
> this getFileID() call is unneccesary when `All.empty() || All.front().getLocation().isFileID()`.
> 
> Worth checking whether bailing out in that case is profitable?
> 
> You could do it right at the top:
> ```
> if (All.size() == 1 || All[0].getLocation().isFileID() != All[1].getLocation().isFileID())
>   return All.take_front(1);
> ```
Good point; I'd say "avoid getFileID" at all costs, even to readability/style.


================
Comment at: clang/lib/Lex/TokenLexer.cpp:1008
+    });
+    assert(!Partition.empty() &&
+           llvm::all_of(Partition.drop_front(),
----------------
hokein wrote:
> sammccall wrote:
> > nickdesaulniers wrote:
> > > Could you just check that all of the tokens in the partition have the same fileID as the first token?
> > > 
> > > ```
> > > FileID FirstFID = SM.getFileID(Partition[0]->getLocation());
> > > llvm::all_of(Partition, [&SM, &FirstID](const Token &T) { return SM.getFileID(T.getLocation() == FID; });
> > > ```
> > > or move the assertion into the take_while above so we iterate less?
> > this assertion seems to belong outside the if() - it applies to both the file/macro case?
> > I'd suggest asserting nonempty first and then the rest as another assertion.
> > also missing an assertion that if there are any more tokens, the next token has a different FileID
> > 
> > that said with these assertions we should probably check we're not regressing debug performance too much!
> The optimization for this case is that we don't call any `getFileID`, the getFileID is only needed in the assert sanity check, so moving the assertion to `take_while` doesn't really work.
> 
> I adjust the code to save some unnecessary `getFileID` call in assert.
Right, I do all development and profiles with Release builds with assertions enabled.  So avoiding getFileID in release+no_asserts builds is a win, but am somewhat bummed to not get as much a win for my rel+assert builds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134942