[optional] Regression in develop

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[optional] Regression in develop

Boost - Dev mailing list
Hi Everyone,
Over the weekend I added changes to Boost.Optional in develop which caused
regression in Boost.Beast and maybe more libraries. I would like to ask for
your input as to how to best approach this.

The change was: for trivial `T` use a different specialization of
`optional<T>` which is trivially-copyable. For this, I need the ability to
declare defaulted functions, and to detect that a given type is trivial.

Boost.TypeTraits do not have `is_trivial` trait, so I compose it form
`has_trivial_move` and the like. But it seems there are compilers for which
Boost.config and Boost.TypeTraits report support for deleted
functions/triviality detection, but where this support is buggy. The
following is a test case in type_traits by Vinnie Falco:
https://github.com/boostorg/type_traits/pull/52/commits/9779157a787620d163308afa45cb94ef42391b32

In addition, the deadline for changes in release 1.66 is getting nigh. I
can see a couple of ways to fix this:

1. Fix Boost.Config and/or Boost.TypeTraits so that they only report
support for the features in question when it is supported without bugs. But
I do not know if there is enough time for this given the deadline.
2. Disable my feature on the compilers with buggy support. But I would need
a help from someone who knows all the compilers and their bugs in
implementation of trivial type traits so that I can put the wight `#ifdef`s.
3. Just revert my changes to Optional, which is also an option, but would
also be a loss.

Any input from Boost experts would be appreciated.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list


On 30/10/2017 07:47, Andrzej Krzemienski via Boost wrote:

> Hi Everyone,
> Over the weekend I added changes to Boost.Optional in develop which caused
> regression in Boost.Beast and maybe more libraries. I would like to ask for
> your input as to how to best approach this.
>
> The change was: for trivial `T` use a different specialization of
> `optional<T>` which is trivially-copyable. For this, I need the ability to
> declare defaulted functions, and to detect that a given type is trivial.
>
> Boost.TypeTraits do not have `is_trivial` trait, so I compose it form
> `has_trivial_move` and the like. But it seems there are compilers for which
> Boost.config and Boost.TypeTraits report support for deleted
> functions/triviality detection, but where this support is buggy. The
> following is a test case in type_traits by Vinnie Falco:
> https://github.com/boostorg/type_traits/pull/52/commits/9779157a787620d163308afa45cb94ef42391b32
>
> In addition, the deadline for changes in release 1.66 is getting nigh. I
> can see a couple of ways to fix this:
>
> 1. Fix Boost.Config and/or Boost.TypeTraits so that they only report
> support for the features in question when it is supported without bugs. But
> I do not know if there is enough time for this given the deadline.

I'm mildly against that in Boost.Config: the issue seems to be VC12,
which certainly does support deleted constructors, the issue is that the
errors generated from their attempted use occurs "late" in the
instantiation cycle so that it doesn't play well with meta-programming. 
Besides, even if we disable deleted functions in our code, that doesn't
stop users instantiating
optional<user_defined_type_with_deleted_constructors> and hitting the
same issue.

In TypeTraits, there is an obvious and trivial workaround for the
std::pair<> case, but other templates may still hit the same issue
unless someone can devise a version of is_default_constructible that
works with VC12 (I don't see an obvious alternative at present).  I
could also make is_default_constructible non-functional for VC12, but
that seems to needlessly harm folks that aren't using deleted constructors.

Is the current issue restricted to std::pairs?

> 2. Disable my feature on the compilers with buggy support. But I would need
> a help from someone who knows all the compilers and their bugs in
> implementation of trivial type traits so that I can put the wight `#ifdef`s.

Seems to just be VC12 based on the CI test results?

> 3. Just revert my changes to Optional, which is also an option, but would
> also be a loss.

Indeed.

Maybe not helping much, yours, John.


---
This email has been checked for viruses by AVG.
http://www.avg.com


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Oct 30, 2017 at 12:47 AM, Andrzej Krzemienski via Boost
<[hidden email]> wrote:
> The following is a test case in type_traits by Vinnie Falco:
> https://github.com/boostorg/type_traits/pull/52/commits/9779157a787620d163308afa45cb94ef42391b32

My idea for a fix is to just specialize
`type_traits::is_default_constructible` for `std::pair<T,U>` where U
is a built-in type, by returning `is_default_constructible<T>`. This
solves the problem for Optional, which invokes
`is_default_constructible` with `std::pair<T, bool>` in this specific
case.

Disclosure: Peter Dimov doesn't like it.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Oct 30, 2017 at 1:59 AM, John Maddock via Boost
<[hidden email]> wrote:
> ...the issue seems to be VC12...

All of the cases where I have seen a compile error happened in clang.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-10-30 10:14 GMT+01:00 Vinnie Falco via Boost <[hidden email]>:

> On Mon, Oct 30, 2017 at 12:47 AM, Andrzej Krzemienski via Boost
> <[hidden email]> wrote:
> > The following is a test case in type_traits by Vinnie Falco:
> > https://github.com/boostorg/type_traits/pull/52/commits/
> 9779157a787620d163308afa45cb94ef42391b32
>
> My idea for a fix is to just specialize
> `type_traits::is_default_constructible` for `std::pair<T,U>` where U
> is a built-in type, by returning `is_default_constructible<T>`. This
> solves the problem for Optional, which invokes
> `is_default_constructible` with `std::pair<T, bool>` in this specific
> case.
>
> Disclosure: Peter Dimov doesn't like it.
>

I think I agree with Peter here. What if somebody is using boost::pair?

I think I will take time to detect all the broken compilers using test
matrix and disable the feature on those compilers.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
On Mon, Oct 30, 2017 at 2:44 AM, Andrzej Krzemienski via Boost
<[hidden email]> wrote:
> I think I will take time to detect all the broken compilers using test
> matrix and disable the feature on those compilers.

Do you mean the feature added to Boost.Optional? This will still leave
Boost.TypeTraits broken for the case I outlined in the test.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
2017-10-30 11:03 GMT+01:00 Vinnie Falco via Boost <[hidden email]>:

> On Mon, Oct 30, 2017 at 2:44 AM, Andrzej Krzemienski via Boost
> <[hidden email]> wrote:
> > I think I will take time to detect all the broken compilers using test
> > matrix and disable the feature on those compilers.
>
> Do you mean the feature added to Boost.Optional? This will still leave
> Boost.TypeTraits broken for the case I outlined in the test.
>

You are correct. Ideally `has_trivial_default_constructor` should be fixed.
But this is something I have no control over, and it is the recent changes
to `optional` that cause regression.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-10-30 9:59 GMT+01:00 John Maddock via Boost <[hidden email]>:

>
>
> On 30/10/2017 07:47, Andrzej Krzemienski via Boost wrote:
>
>> Hi Everyone,
>> Over the weekend I added changes to Boost.Optional in develop which caused
>> regression in Boost.Beast and maybe more libraries. I would like to ask
>> for
>> your input as to how to best approach this.
>>
>> The change was: for trivial `T` use a different specialization of
>> `optional<T>` which is trivially-copyable. For this, I need the ability to
>> declare defaulted functions, and to detect that a given type is trivial.
>>
>> Boost.TypeTraits do not have `is_trivial` trait, so I compose it form
>> `has_trivial_move` and the like. But it seems there are compilers for
>> which
>> Boost.config and Boost.TypeTraits report support for deleted
>> functions/triviality detection, but where this support is buggy. The
>> following is a test case in type_traits by Vinnie Falco:
>> https://github.com/boostorg/type_traits/pull/52/commits/9779
>> 157a787620d163308afa45cb94ef42391b32
>>
>> In addition, the deadline for changes in release 1.66 is getting nigh. I
>> can see a couple of ways to fix this:
>>
>> 1. Fix Boost.Config and/or Boost.TypeTraits so that they only report
>> support for the features in question when it is supported without bugs.
>> But
>> I do not know if there is enough time for this given the deadline.
>>
>
> I'm mildly against that in Boost.Config: the issue seems to be VC12, which
> certainly does support deleted constructors, the issue is that the errors
> generated from their attempted use occurs "late" in the instantiation cycle
> so that it doesn't play well with meta-programming.  Besides, even if we
> disable deleted functions in our code, that doesn't stop users
> instantiating optional<user_defined_type_with_deleted_constructors> and
> hitting the same issue.
>
> In TypeTraits, there is an obvious and trivial workaround for the
> std::pair<> case, but other templates may still hit the same issue unless
> someone can devise a version of is_default_constructible that works with
> VC12 (I don't see an obvious alternative at present).  I could also make
> is_default_constructible non-functional for VC12, but that seems to
> needlessly harm folks that aren't using deleted constructors.
>

Maybe provide a yet another type trait, like
`is_sfinae_friendly_default_constructible`?
Or, maybe provide a macro that would tell if on this platform I get a 100%
correct behavior on `is_default_constructible` and
`has_trivial_default_constructor`?`

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Andrzej Krzemienski wrote:

> In addition, the deadline for changes in release 1.66 is getting nigh.

The deadline for breaking changes has already passed though, and...

> 1. Fix Boost.Config and/or Boost.TypeTraits so that they only report
> support for the features in question when it is supported without bugs.
> But I do not know if there is enough time for this given the deadline.

... this will almost certainly be a breaking change.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
John Maddock wrote:

> I'm mildly against that in Boost.Config: the issue seems to be VC12, ...

No. The suggested addition in
https://github.com/boostorg/type_traits/pull/52/commits/9779157a787620d163308afa45cb94ef42391b32 
is wrong; there's nothing type_traits can do about it because instantiating
the constructor is not an immediate context and not subject to SFINAE.

The problem is in has_trivial_constructor<pair<...>> and is caused by the
following logic in boost/type_traits/has_trivial_constructor.hpp:

#if (defined(__GNUC__) && (__GNUC__ * 100 + __GNUC_MINOR__ >= 409)) ||
defined(BOOST_CLANG) || (defined(__SUNPRO_CC) &&
defined(BOOST_HAS_TRIVIAL_CONSTRUCTOR))
#include <boost/type_traits/is_default_constructible.hpp>
#define BOOST_TT_TRIVIAL_CONSTRUCT_FIX && is_default_constructible<T>::value
#else

which is then used in

template <typename T> struct has_trivial_constructor
#ifdef BOOST_HAS_TRIVIAL_CONSTRUCTOR
   : public integral_constant <bool, ((::boost::is_pod<T>::value ||
BOOST_HAS_TRIVIAL_CONSTRUCTOR(T)) BOOST_TT_TRIVIAL_CONSTRUCT_FIX)>{};
#else

That is, the issue only occurs when (a) the compiler is gcc >= 4.9, clang,
Sun, (b) the compiler has intrinsic support for __has_trivial_constructor,
(c) instantiating the type's default constructor is a hard error.

The latter is the case for pair<deleted-constructor, int> before the change
in the standard that made its default constructor SFINAE-friendly.

So in practice, this manifests on Travis on gcc 4.9 and clang using
libstdc++ before 5.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
2017-10-30 13:02 GMT+01:00 Peter Dimov via Boost <[hidden email]>:

> John Maddock wrote:
>
> I'm mildly against that in Boost.Config: the issue seems to be VC12, ...
>>
>
> No. The suggested addition in https://github.com/boostorg/ty
> pe_traits/pull/52/commits/9779157a787620d163308afa45cb94ef42391b32 is
> wrong; there's nothing type_traits can do about it because instantiating
> the constructor is not an immediate context and not subject to SFINAE.
>
> The problem is in has_trivial_constructor<pair<...>> and is caused by the
> following logic in boost/type_traits/has_trivial_constructor.hpp:
>
> #if (defined(__GNUC__) && (__GNUC__ * 100 + __GNUC_MINOR__ >= 409)) ||
> defined(BOOST_CLANG) || (defined(__SUNPRO_CC) &&
> defined(BOOST_HAS_TRIVIAL_CONSTRUCTOR))
> #include <boost/type_traits/is_default_constructible.hpp>
> #define BOOST_TT_TRIVIAL_CONSTRUCT_FIX && is_default_constructible<T>::v
> alue
> #else
>
> which is then used in
>
> template <typename T> struct has_trivial_constructor
> #ifdef BOOST_HAS_TRIVIAL_CONSTRUCTOR
>   : public integral_constant <bool, ((::boost::is_pod<T>::value ||
> BOOST_HAS_TRIVIAL_CONSTRUCTOR(T)) BOOST_TT_TRIVIAL_CONSTRUCT_FIX)>{};
> #else
>
> That is, the issue only occurs when (a) the compiler is gcc >= 4.9, clang,
> Sun, (b) the compiler has intrinsic support for __has_trivial_constructor,
> (c) instantiating the type's default constructor is a hard error.
>

Is the above a conjunction of (a) and (b) and (c)?


> The latter is the case for pair<deleted-constructor, int> before the
> change in the standard that made its default constructor SFINAE-friendly.
>

This sounds like the trait works fine and it is just implementations of
`std::pair` that are not SFINAE friendly.


> So in practice, this manifests on Travis on gcc 4.9 and clang using
> libstdc++ before 5.
>

You mean version of  libstdc++ is before 5?

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
Andrzej Krzemienski wrote:
> 2017-10-30 13:02 GMT+01:00 Peter Dimov via Boost <[hidden email]>:
> > That is, the issue only occurs when (a) the compiler is gcc >= 4.9,
> > clang, Sun, (b) the compiler has intrinsic support for
> > __has_trivial_constructor, (c) instantiating the type's default
> > constructor is a hard error.
>
> Is the above a conjunction of (a) and (b) and (c)?

It is.

> This sounds like the trait works fine and it is just implementations of
> `std::pair` that are not SFINAE friendly.

In principle the issue should not be pair-specific and should affect all
kinds of user classes, but in practice I've trouble reproducing it with
anything else besides std::pair. No idea yet what specifically in std::pair
causes it.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Oct 30, 2017 at 7:53 AM, Peter Dimov via Boost <
[hidden email]> wrote:

> Andrzej Krzemienski wrote:
>
> In addition, the deadline for changes in release 1.66 is getting nigh.
>>
>
> The deadline for breaking changes has already passed though, and...
>
> 1. Fix Boost.Config and/or Boost.TypeTraits so that they only report
>> support for the features in question when it is supported without bugs. But
>> I do not know if there is enough time for this given the deadline.
>>
>
> ... this will almost certainly be a breaking change.
>
>
My builds on develop using MSVC 2010 in Appveyor are broken.  I assume this
is related?

https://ci.appveyor.com/project/jeking3/uuid/build/1.0.120-develop/job/1tpo35ce766kmff5#L762


.\boost/optional/optional.hpp(944) : error C2253: 'optional<T>' : pure
specifier or abstract override specifier only allowed on virtual function
.\boost/optional/optional.hpp(1339) : see reference to class template
instantiation 'boost::optional<T>' being compiled
<https://ci.appveyor.com/project/jeking3/uuid/build/1.0.120-develop/job/1tpo35ce766kmff5#L764>
.\boost/optional/optional.hpp(954) : error C2253: 'optional<T>' : pure
specifier or abstract override specifier only allowed on virtual function

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [optional] Regression in develop

Boost - Dev mailing list
2017-11-06 20:55 GMT+01:00 James E. King, III via Boost <
[hidden email]>:

> On Mon, Oct 30, 2017 at 7:53 AM, Peter Dimov via Boost <
> [hidden email]> wrote:
>
> > Andrzej Krzemienski wrote:
> >
> > In addition, the deadline for changes in release 1.66 is getting nigh.
> >>
> >
> > The deadline for breaking changes has already passed though, and...
> >
> > 1. Fix Boost.Config and/or Boost.TypeTraits so that they only report
> >> support for the features in question when it is supported without bugs.
> But
> >> I do not know if there is enough time for this given the deadline.
> >>
> >
> > ... this will almost certainly be a breaking change.
> >
> >
> My builds on develop using MSVC 2010 in Appveyor are broken.  I assume this
> is related?
>
> https://ci.appveyor.com/project/jeking3/uuid/build/1.0.120-develop/job/
> 1tpo35ce766kmff5#L762
>
>
> .\boost/optional/optional.hpp(944) : error C2253: 'optional<T>' : pure
> specifier or abstract override specifier only allowed on virtual function
> .\boost/optional/optional.hpp(1339) : see reference to class template
> instantiation 'boost::optional<T>' being compiled
> <https://ci.appveyor.com/project/jeking3/uuid/build/1.0.120-develop/job/
> 1tpo35ce766kmff5#L764>
> .\boost/optional/optional.hpp(954) : error C2253: 'optional<T>' : pure
> specifier or abstract override specifier only allowed on virtual function
>
>
This should be fixed now. Sorry for the trouble.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost