This is an archive of the discontinued LLVM Phabricator instance.

Refactoring, update MacroInfo interface so a for range can be used to iterate through the tokens
ClosedPublic

Authored by danielmarjamaki on Apr 17 2015, 7:19 AM.

Diff Detail

Event Timeline

danielmarjamaki retitled this revision from to Refactoring, update MacroInfo interface so a for range can be used to iterate through the tokens.
danielmarjamaki updated this object.
danielmarjamaki edited the test plan for this revision. (Show Details)

what is your opinion on this refactoring?

alexfh added a subscriber: Unknown Object (MLST).Apr 17 2015, 7:25 AM

thanks for adding cfe-commits.. I did not see how to do it. I could add you but not cfe-commits.

alexfh edited edge metadata.Apr 17 2015, 7:36 AM

Hi Daniel,

Please always add cfe-commits to CC for all code reviews touching cfe or clang-tools-extra repositories.

what is your opinion on this refactoring?

Seems to make things better. A couple of comments inline.

Thanks!

include/clang/Lex/MacroInfo.h
244

This should return const SmallVectorImpl<Token> & to avoid dependency on the SmallVector inline storage size.

lib/Frontend/PrintPreprocessedOutput.cpp
67

There's no need in the clang:: qualifier here due to the using directive above.

danielmarjamaki edited edge metadata.

good comments. I have taken care of those.

Use ArrayRef after comment in cfe-commits

Regenerate diff with more context

rsmith added a subscriber: rsmith.Apr 28 2015, 2:24 PM

question: is ArrayRef faster/safer/cleaner somehow?

Yes:

  1. ArrayRef leaks fewer implementation details to the caller; it's just a guaranteed-to-be-contiguous sequence.
  2. The reason for dealing in SmallVectorImpls is to mutate them. Using SmallVectorImpl for read-only access is surprising.
  3. Range accessors are usually assumed to provide a shallow view of some external data. In particular, it's not unreasonable to assume auto T = MI->tokens(); to be cheap (which it is for ArrayRef and not for SmallVectorImpl).
tools/extra/modularize/PreprocessorTracker.cpp
408–409

Use auto here to remove the risk of creating a temporary? I think it's sufficiently clear that you get Tokens from tokens().

Thank you for reviewing. I now use auto.

rsmith added inline comments.Apr 29 2015, 10:37 AM
lib/Frontend/PrintPreprocessedOutput.cpp
67–68

Sorry for not being more explicit, I meant that you should replace just the clang::Token with auto. This now *always* makes a copy. const auto & is the right thing to use here.

Don't make copies in for loops

alexfh added a comment.May 8 2015, 3:19 AM

Sorry for the delay. I was on vacation.

Please address the comment and this should be fine to submit unless Richard has any comments.

include/clang/Lex/MacroInfo.h
244

ArrayRef is pretty const on its own (in the sense that it doesn't allow modification of the elements in the range), so an extra const before ArrayRef<...> is not needed here.

This revision was automatically updated to reflect the committed changes.