[PATCH] Adding a specific_attribute_reverse_iterator

Clang Frontend
1

Hi all,
the current Clang implementation has a specific_attr_iterator 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).

specific_attr_reverse_iterator.diff (5.19 KB)

2

I hate to say it, but trying to solve the attributes-on-types problem this way seems like a bad idea.

int [[attr1]] * [[attr2, attr3]] var;
int [[attr1, attr2]] * [[attr3]] var;

We should be able to distinguish those, but we don’t. And this won’t help us do that, and pretending that it does is not a good thing, IMHO.

Jordan

P.S. At the micro level, what’s wrong with using NULL as the reverse_iterator sentinel?

3

Indeed, the reverse iterator is not intended as a solution to a problem, but as useful functionality in the absence of a solution (and because the solution is likely to take a long time to be implemented). Also, is there a disadvantage in having the reverse iterator functionality?

At the micro level, NULL is not a valid value for objects of type std::reverse_iterator, so the compiler would not compile the code with RNull replaced by NULL.

Cheers!
Alex

4

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