hokein added inline comments.
================
Comment at: clang/lib/Lex/TokenLexer.cpp:1019
+ // sourcelocation-against-bounds comparison.
+ FileID BeginFID = SM.getFileID(BeginLoc);
+ SourceLocation Limit =
----------------
nickdesaulniers wrote:
> 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.
We have handled the 1-element special case in the caller, so All.size() > 1 in
this function. Added an assertion.
> All[0].getLocation().isFileID() != All[1].getLocation().isFileID()
I tried it when writing this patch, I don't find the result now, but IIRC
performance difference is negligible. I'm happy to add this special case since
it won't hurt the readability too much.
================
Comment at: clang/lib/Lex/TokenLexer.cpp:1038
+ for (Token& T : Partition) {
+ SourceLocation::IntTy RelativeOffset = T.getLocation().getRawEncoding() -
+ BeginLoc.getRawEncoding();
----------------
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
```
================
Comment at: clang/lib/Lex/TokenLexer.cpp:1008
+ });
+ assert(!Partition.empty() &&
+ llvm::all_of(Partition.drop_front(),
----------------
nickdesaulniers wrote:
> 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.
> this assertion seems to belong outside the if() - it applies to both the
> file/macro case?
yeah, it should apply the `else` case. Moved it outside -- it seems unnecessary
to check the `else` case, since we actually do the partition based on the file
id (that being said, it is somehow like `int s = 1+1; assert(s == 2);`), but
it might be good for code readability.
> I do all development and profiles with Release builds with assertions enabled
hmm, I think when doing profiles we probably should use a release build without
assertions enabled to give a more correct result, since assertion will slow
everything down.
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
