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