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

[Haojian Wu via Phabricator via cfe-commits]

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