[libcxx] r203587 - Implement LWG 2360: 'reverse_iterator::operator*() is unimplementable'. Note that this is a (small) behavior change in the library. Reverse iterators whose base iterators' operator* return references to 'within themselves' have been sacrificed to the greater goal of avoiding data races.

[Richard Smith]

On Tue, Mar 11, 2014 at 5:16 PM, John McCall <rjmccall at apple.com> wrote:

> On Mar 11, 2014, at 2:56 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Tue, Mar 11, 2014 at 2:41 PM, Marshall Clow <mclow.lists at gmail.com>wrote:
>
>>
>> On Mar 11, 2014, at 2:14 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> I'm open to suggestions here:
>>> a) I can put the instance variable back, and never use it. No ABI break,
>>> but an unused iterator in every reverse iterator.
>>>
>>
>> Any code that passes a reverse_iterator between TUs built against
>> different libc++ versions would still not work.
>>
>>
>> Ok, I missed this comment; I thought it applied to the change - not the
>> proposed fix.
>> I disagree. I do not believe that putting the instance variable back and
>> not using it will not break code across TUs.
>>
>> The only use of the instance variable __t is in the operator*() for the
>> reverse iterator, which used to say:
>>  _LIBCPP_INLINE_VISIBILITY reference operator*() const {__t = current;
>> return *--__t;}
>> (i.e, the variable is set immediately before use)
>> but now says:
>> _LIBCPP_INLINE_VISIBILITY reference operator*() const {_Iter __tmp =
>> current; return *--__tmp;}
>>
>> The purpose of having this instance variable "stick around", rather than
>> being a local variable, was to support iterators whose operator* returned a
>> reference to something inside of the iterator. If the iterator was a local
>> variable, then the things contained inside of it would be destroyed when
>> the iterator was destroyed.
>>
>> However, in #2360, STL at microsoft.com pointed out that this was inviting
>> silent data races, and the LWG decided not to support those kind of
>> iterators any more.
>> (see the comments on LWG issue #198, which #2360 partially reversed).
>>
>> Richard -- clearly you think otherwise.
>> I'd like to hear your explanation as to why.
>>
>
> On further study, I think the approach you're talking about is fine. You'd
> need to keep the actual instance variable, not just storage appropriate for
> it (you still need to call the default constructor and destructor, if any),
> and you'll probably need it to still be 'mutable' to avoid more subtle
> changes.
>
> (... and we should remove that member in the 'unstable ABI' mode when we
> have it!)
>
>
> For what it's worth, I've been planning to submit a patch soon which would
> allow data-structure-specific ABI versioning in order to help deal with the
> std::pair problem, among others, along the lines of what Richard suggested
> except leaving unaffected data structures at version __1.
>
> I would propose that libc++ should default to ABI instability (except,
> obviously, with its own dynamic library), but that a user should be able to
> simply pass -D_LIBCPP_ABI_VERSION=3 and get reliable interoperability.
>
> I was thinking something along the lines of:
>
>
>
> /* __config */
>
> // ABI version log:
> //   1 - The beginning of time.
> //   2 - A std::pair of trivially copyable types should be trivially
> copyable.
> //   3 - ...
> #ifndef _LIBCPP_ABI_VERSION
> #define _LIBCPP_ABI_VERSION 3   // bump the current number
> #endif
>
> #ifndef _LIBCPP_PAIR_ABI_VERSION
> #if _LIBCPP_ABI_VERSION >= 2
> #define _LIBCPP_PAIR_ABI_VERSION 2
> #else
> #define _LIBCPP_PAIR_ABI_VERSION 1
> #endif
> #endif
>
> // ...more clauses here as ABI changes warrant...
>
> // Everything that hasn't had an ABI revision.
> #ifndef _LIBCPP_DEFAULT_ABI_VERSION
> #define _LIBCPP_DEFAULT_ABI_VERSION 1
> #endif
>
> ...
>
> #define _LIBCPP_BEGIN_NAMESPACE_STD(VERSION) namespace std {inline
> namespace _LIBCPP_NAMESPACE(VERSION) {
> #define _LIBCPP_BEGIN_NAMESPACE_STD_DEFAULT
> _LIBCPP_BEGIN_NAMESPACE_STD(_LIBCPP_DEFAULT_ABI_VERSION)
>
> ...
>
>
>
> /* utility */
>
> _LIBCPP_BEGIN_NAMESPACE_STD_DEFAULT
> /* everything up to std::pair */
> _LIBCPP_END_NAMESPACE_STD
>
> _LIBCPP_BEGIN_NAMESPACE_STD(_LIBCPP_PAIR_ABI_VERSION)
> template <class T, class U> class pair {
> #if _LIBCPP_PAIR_ABI_VERSION < 2
>   /* copy constructor etc. */
> #endif
> };
> _LIBCPP_END_NAMESPACE_STD
>
>
>
> Thoughts?  Better ideas for the main version sequence?
>

Can we do without the separate _LIBCPP_ABI_VERSION and
_LIBCPP_PAIR_ABI_VERSION? Something like:

_LIBCPP_BEGIN_NAMESPACE_STD_DEFAULT
// ...
_LIBCPP_END_NAMESPACE_STD

_LIBCPP_BEGIN_NAMESPACE_STD_VERSION(1, 2)
template <class T, class U> class pair {
#if _LIBCPP_ABI_VERSION < 2
// ...
#endif
// ...
};
_LIBCPP_END_NAMESPACE_STD

... where the _VERSION macro is passed the set of ABI versions where pair
changed, and it picks the latest supplied number that is <=
_LIBCPP_ABI_VERSION.

We'll need to be careful to version-bump everything that depends on 'pair'
when we version-bump 'pair'. It'd be nice if we had some enforcement that a
__1 name can't use a >= __2 name.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140311/1c4493ea/attachment.html>