[cfe-dev] Fwd: [PATCH] Adding a specific_attribute_reverse_iterator

[Philip Reames]

At the high level, I am opposed to this change.  As Jordan said, trying 
to mask over a problem rather than fixing it is a bad idea.  This type 
of "fix" could create a nightmare of legacy behavior when users 
accidentally start relying on ordering.

Is there a bug somewhere for this issue?  I've spent a fair amount of 
time in the attribute handling code recently and was unaware of it.

Specifically to your point about RNull, the use of a distinguished value 
doesn't seem too evil to me provided it provides all the standard 
iterator functionality (!=).

Given that walking over the set of attributes without checking whether 
its empty is an exceedingly common pattern in our code, I do not think 
the created on demand empty attribute is a good idea.

An alternate design would be to have a single static empty attributes 
array and return an reverse iterator to the beginning and end of that. 
That has some nasty implications for walking an data structure while 
mutating it.  While that may be fine by the spec - note may, I haven't 
checked - it would be a debugging disaster.

p.s. Thank you for looking into this and starting a conversation. 
Please keep pushing, maybe we can get the underlying issue fixed.

Yours,
Philip Reames

On 11/14/2012 02:00 PM, Alexandros Tzannes wrote:
>   Hi all,
> the current Clang implementation has a specific_attr_iterator<AttrType>
> that allows iterating over the  attributes of a certain type (all
> attributes are kept in a single llvm::SmallVector), but it lacks a
> reverse iterator. This patch adds this functionality.
>
> Why is this useful/needed (in the short run).
> -----------------------------------------------------------
> The C++11 standard allows attributes to appertain to types and portions
> of the declarator, e.g.:
> int[[attr1] *[[attr2] *[[attr3] var;
>
> However, Clang is still collapsing those attributes on the declaration
> itself:
> int **var [[attr1,attr2,attr3]].
>
> According to C++11 when multiple attributes of the same kind appertain
> to the same "node", their order is irrelevant (note that these
> attributes may have different attribute parameters):
>
> int var [[attrA("x"), attrA("y")]]
> and
> int var [[attrA("y"), attrA("x")]]
>
> must mean the same thing.
>
> However, until attributes are allowed to appertain to all the new
> locations described by C++11, it would be useful to use their order to
> "map" them onto the  different nodes they should appertain. The
> specific_attr_reverse_iterator provides some functionality towards that
> goal.
>
> Implementation
> ----------------------
> The implementation is a slightly modified version of the code of the
> corresponding forward iterator. A small difference is that the forward
> iterator returns a pointer to the underlying type *T, whereas the
> reverse one returns an object of type typedef
> std::reverse_iterator<iterator
> <http://llvm.org/docs/doxygen/html/classllvm_1_1SmallVectorTemplateCommon.html#a309d93eaafb8f5a8dfb7de2a231335ea>>.
> For that reason, Decl::attr_rbegin() and Decl::attr_rend() for
> attr_reverse_iterator check if the Decl has attributes, and if so,
> return the begin or the end; otherwise, they return a sentinel RNull,
> which is implemented as a static member of Decl.
>
> Using the RNull sentinel is a bit awkward. Two alternatives are the
> following:
> 1. Make the user responsible of checking if a Decl has an attributes
> vector before trying to get a specific_attr_reverse_iterator. Trying to
> get such an iterator on a Decl without any attrs will generate an
> assertion failure. This approach gets rid of RNull, but requires users
> to treat fwd and reverse iterators differently.
>
> 2. Each time the user tries to get a reverse iterator on a Decl without
> attributes, create an empty attribute vector and return the beginning
> (or the end) of it. This approach also removes the awkward RNull, but
> may create many unneeded empty attribute vectors.
>
> Please let me know if one of these alternatives is preferable.
>
> Cheers!
> Alex Tzannes
>
> [[written during the Hacker Lab at last week's LLVM devellopers' meeting]]
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>