On Mar 11, 2014, at 6:12 PM, Richard Smith <[email protected]> wrote:
> On Tue, Mar 11, 2014 at 5:16 PM, John McCall <[email protected]> wrote:
> On Mar 11, 2014, at 2:56 PM, Richard Smith <[email protected]> wrote:
>> On Tue, Mar 11, 2014 at 2:41 PM, Marshall Clow <[email protected]> wrote:
>>
>> On Mar 11, 2014, at 2:14 PM, Richard Smith <[email protected]> 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, [email protected] 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.
I don’t think that macro is possible to write without something like Boost
Preprocessor (which is way too heavyweight to inflict on every user of the
standard library). Remember that we need it to token-expand to a literal, not
just fold to a constant.
But you’re right that we neither need nor want to support mixing and matching
different ABI versions of different data structures, which at least removes
some #ifndef checks.
> We'll need to be careful to version-bump everything that depends on 'pair'
> when we version-bump ‘pair’.
Yeah, that might make this annoying. On the other hand, for a lot of things
like std::map, we can just use _LIBCPP_PAIR_ABI_VERSION until there are
multiple ABI breaks in play for that data structure, which is an argument in
favor of actually having those macros. We could move them into the appropriate
headers, though.
I think we actually *can’t* just bump the version across the board; there are
symbols in the __1 namespace that are only defined in the shared library (and
not just explicit instantiations).
> It'd be nice if we had some enforcement that a __1 name can't use a >= __2
> name.
That seems like a reasonable tool for somebody else to whip up. :)
John.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
