[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.

[John McCall]

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?

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140311/77dd294d/attachment.html>