[clang] RFC: NFC: simplify macro tokens assignment

[zhouyizhou]

Summary

I think we could simplify Tokens = &*Macro->tokens_begin() in clang::TokenLexer::Init.
Because the type of Tokens is const class clang::Token *, the return type of Macro->tokens_begin() is also class clang::Token *, so I think the &* sequence can be removed
Thanks for you time

Thanks
Zhouyi

Signed-off-by: Zhouyi Zhou <[e-mail]>

[simon_tatham]

I don't think so, I'm afraid. If you look at the definition of MacroInfo::tokens_begin() in clang/include/clang/Lex/MacroInfo.h, you see that it doesn't return a raw Token *: it returns a C++ iterator object.

using tokens_iterator = SmallVectorImpl<Token>::const_iterator;

tokens_iterator tokens_begin() const { return ReplacementTokens.begin(); }

So you do have to dereference the iterator to get a Token &, and then address-take that to turn it into a Token *.

[zhouyizhou]

In D115014#3169400, @simon_tatham wrote:

I don't think so, I'm afraid. If you look at the definition of MacroInfo::tokens_begin() in clang/include/clang/Lex/MacroInfo.h, you see that it doesn't return a raw Token *: it returns a C++ iterator object.

using tokens_iterator = SmallVectorImpl<Token>::const_iterator;

tokens_iterator tokens_begin() const { return ReplacementTokens.begin(); }

So you do have to dereference the iterator to get a Token &, and then address-take that to turn it into a Token *.

Thank Simon for reviewing my patch ;-)

In llvm/include/llvm/ADT/SmallVector.h, I found this: using const_iterator = const T *;

also I use gdb to print the return type of tokens_begin()
(gdb) ptype const_iterator
type = const class clang::Token {

...
} *

So, there still remains little puzzle in my head ;-)

Thanks again
Zhouyi

[simon_tatham]

Ah, now I see what you mean – I didn't look far enough!

I don't know this code well (in fact I'm not sure why you picked me as a reviewer), but off the top of my head: the question is not just whether tokens_iterator happens to be the same thing as const Token * now. It's whether it's guaranteed to stay that way in the future.

One of the purposes of these iterator types is that they form an opaque abstraction layer: the client uses the type in only the ways that are guaranteed to work by the iterator specification, and then the implementation can completely change without breaking any client.

If you make a change like this, and later SmallVector::iterator changes to some other legal implementation, or tokens_iterator changes to be something other than a SmallVector::iterator, then this change will have to be reverted.

[zhouyizhou]

In D115014#3169444, @simon_tatham wrote:

Ah, now I see what you mean – I didn't look far enough!

I don't know this code well (in fact I'm not sure why you picked me as a reviewer), but off the top of my head: the question is not just whether tokens_iterator happens to be the same thing as const Token * now. It's whether it's guaranteed to stay that way in the future.

One of the purposes of these iterator types is that they form an opaque abstraction layer: the client uses the type in only the ways that are guaranteed to work by the iterator specification, and then the implementation can completely change without breaking any client.

If you make a change like this, and later SmallVector::iterator changes to some other legal implementation, or tokens_iterator changes to be something other than a SmallVector::iterator, then this change will have to be reverted.

Thank Simon for resolve my confusion

I am crystal clear about the use of &* pair now. I did benefit a lot.

I pick you as a reviewer because you have committed to TokenLexer.cpp recently (I use git log along with git blame );-)

Thanks again
Zhouyi

clang/lib/Lex/TokenLexer.cpp

void TokenLexer::Init(Token &Tok, SourceLocation ELEnd, MacroInfo *MI,
   ActualArgs = Actuals;
   CurTokenIdx = 0;
 
   ExpandLocStart = Tok.getLocation();
   ExpandLocEnd = ELEnd;
   AtStartOfLine = Tok.isAtStartOfLine();
   HasLeadingSpace = Tok.hasLeadingSpace();
   NextTokGetsSpace = false;
-  Tokens = &*Macro->tokens_begin();
+  Tokens = Macro->tokens_begin();
   OwnsTokens = false;
   DisableMacroExpansion = false;
   IsReinject = false;
   NumTokens = Macro->tokens_end()-Macro->tokens_begin();
   MacroExpansionStart = SourceLocation();
 
   SourceManager &SM = PP.getSourceManager();
   MacroStartSLocOffset = SM.getNextLocalOffset();