SV: [PATCH] Refactor MacroInfo for easier access in for-loops

[Daniel Marjamäki]

Hello!

Imho the ArrayRef seems more explicit about what the args() returns. The Iterator_Range obfuscates the interface.

Personally I don't see why we should prevent that people use the ArrayRef interface.

Maybe somebody would want to access arguments outside for loops also.

    const auto &Args = X.args();
    ...
    SZ = Args.size();
    II = Args[0];
    ...

I see nothing wrong with that.

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden

Mobile:                 +46 (0)709 12 42 62
E-mail:                 Daniel.Marjamaki at evidente.se

www.evidente.se

________________________________________
Från: Justin Bogner [justin at justinbogner.com] för Justin Bogner [mail at justinbogner.com]
Skickat: den 27 maj 2015 05:53
Till: Alexander Kornienko
Cc: Sebastian Edman; Daniel Marjamäki; reviews+D9934+public+52cdd47a557ebec6 at reviews.llvm.org; cfe-commits at cs.uiuc.edu Commits
Ämne: Re: [PATCH] Refactor MacroInfo for easier access in for-loops

Alexander Kornienko <alexfh at google.com> writes:
> On Fri, May 22, 2015 at 10:23 PM, Justin Bogner <mail at justinbogner.com> wrote:
>> Sebastian Edman <sebastian.edman at evidente.se> writes:
>>> Hi alexfh, danielmarjamaki,
>>>
>>> Added possibilty to extract the arguments in a MacroInfo as a
>>> container in order to use C++11 style for loops.
>>>
>>> http://reviews.llvm.org/D9934
>>>
>>> Files:
>>>   include/clang/Lex/MacroInfo.h
>>>   lib/Lex/PPMacroExpansion.cpp
>>>   lib/Serialization/ASTWriter.cpp
>>>
>>> Index: include/clang/Lex/MacroInfo.h
>>> ===================================================================
>>> --- include/clang/Lex/MacroInfo.h
>>> +++ include/clang/Lex/MacroInfo.h
>>> @@ -182,6 +182,9 @@
>>>    bool arg_empty() const { return NumArguments == 0; }
>>>    arg_iterator arg_begin() const { return ArgumentList; }
>>>    arg_iterator arg_end() const { return ArgumentList+NumArguments; }
>>> +  ArrayRef<IdentifierInfo*> args() const {
>>> +       return ArrayRef<IdentifierInfo*>(ArgumentList, NumArguments);
>>> +  }
>>
>> It's probably better to use iterator_range from llvm/ADT/iterator_range.h.
>
> Can you explain why you think it's better than ArrayRef here?

Primarily consistency, familiarity, and information hiding.

- As long as this is only used for range-for, it'll likely be identical
  code generated and whoever's using this shouldn't know the difference.

- Using iterator_range prevents uses such as random access, which limit
  how the API can change without needing to change clients.

- If we do want random access, llvm and clang would generally spell it
  getArg(i) rather than args()[i], based on existing code.