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

[danielmarjamaki]

Summary

This is just for code cleanup purposes. No functional change is intended.

Let me know if you like this or not.

[danielmarjamaki]

what is your opinion on this refactoring?

[danielmarjamaki]

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

[alexfh]

Hi Daniel,

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

In D9079#157602, @danielmarjamaki wrote:

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–68

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

[danielmarjamaki]

good comments. I have taken care of those.

[danielmarjamaki]

Use ArrayRef after comment in cfe-commits

[danielmarjamaki]

Regenerate diff with more context

[rsmith]

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().

[danielmarjamaki]

Thank you for reviewing. I now use auto.

[rsmith]

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.

[danielmarjamaki]

Don't make copies in for loops

[alexfh]

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.

include/clang/Lex/MacroInfo.h

const Token &getReplacementToken(unsigned Tok) const {
     assert(Tok < ReplacementTokens.size() && "Invalid token #");
     return ReplacementTokens[Tok];
   }
 
   typedef SmallVectorImpl<Token>::const_iterator tokens_iterator;
   tokens_iterator tokens_begin() const { return ReplacementTokens.begin(); }
   tokens_iterator tokens_end() const { return ReplacementTokens.end(); }
   bool tokens_empty() const { return ReplacementTokens.empty(); }
+  const SmallVectorImpl<Token> &tokens() const { return ReplacementTokens; }
 
   /// \brief Add the specified token to the replacement text for the macro.
   void AddTokenToBody(const Token &Tok) {
     assert(!IsDefinitionLengthCached &&
           "Changing replacement tokens after definition length got calculated");
     ReplacementTokens.push_back(Tok);
   }
 

lib/Frontend/PrintPreprocessedOutput.cpp

static void PrintMacroDefinition(const IdentifierInfo &II, const MacroInfo &MI,
   }
 
   // GCC always emits a space, even if the macro body is empty.  However, do not
   // want to emit two spaces if the first token has a leading space.
   if (MI.tokens_empty() || !MI.tokens_begin()->hasLeadingSpace())
     OS << ' ';
 
   SmallString<128> SpellingBuffer;
-  for (MacroInfo::tokens_iterator I = MI.tokens_begin(), E = MI.tokens_end();
-       I != E; ++I) {
-    if (I->hasLeadingSpace())
+  for (const Token &T : MI.tokens()) {
+    if (T.hasLeadingSpace())
       OS << ' ';
 
-    OS << PP.getSpelling(*I, SpellingBuffer);
+    OS << PP.getSpelling(T, SpellingBuffer);
   }
 }
 
 //===----------------------------------------------------------------------===//
 // Preprocessed token printer
 //===----------------------------------------------------------------------===//
 
 namespace {

tools/extra/modularize/PreprocessorTracker.cpp

 // consistency checking, although it displays the incorrect
 // expansion in error messages.
 static std::string getMacroExpandedString(clang::Preprocessor &PP,
                                           llvm::StringRef MacroName,
                                           const clang::MacroInfo *MI,
                                           const clang::MacroArgs *Args) {
   std::string Expanded;
   // Walk over the macro Tokens.
-  typedef clang::MacroInfo::tokens_iterator Iter;
-  for (Iter I = MI->tokens_begin(), E = MI->tokens_end(); I != E; ++I) {
-    clang::IdentifierInfo *II = I->getIdentifierInfo();
+  for (const clang::Token &T : MI->tokens()) {
+    clang::IdentifierInfo *II = T.getIdentifierInfo();
     int ArgNo = (II && Args ? MI->getArgumentNum(II) : -1);
     if (ArgNo == -1) {
       // This isn't an argument, just add it.
       if (II == nullptr)
-        Expanded += PP.getSpelling((*I)); // Not an identifier.
+        Expanded += PP.getSpelling(T); // Not an identifier.
       else {
         // Token is for an identifier.
         std::string Name = II->getName().str();
         // Check for nexted macro references.
         clang::MacroInfo *MacroInfo = PP.getMacroInfo(II);
         if (MacroInfo)
           Expanded += getMacroExpandedString(PP, Name, MacroInfo, nullptr);
         else