[optional] Regression in develop
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/9779157a787620d16330... 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;
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/9779157a787620d16330...
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
2017-10-30 9:59 GMT+01:00 John Maddock via Boost <boost@lists.boost.org>:
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;
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/9779157a787620d16330... 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.
2017-10-30 13:02 GMT+01:00 Peter Dimov via Boost <boost@lists.boost.org>:
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;
Andrzej Krzemienski wrote:
2017-10-30 13:02 GMT+01:00 Peter Dimov via Boost <boost@lists.boost.org>:
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.
On Mon, Oct 30, 2017 at 12:47 AM, Andrzej Krzemienski via Boost <boost@lists.boost.org> wrote:
The following is a test case in type_traits by Vinnie Falco: https://github.com/boostorg/type_traits/pull/52/commits/9779157a787620d16330...
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.
2017-10-30 10:14 GMT+01:00 Vinnie Falco via Boost <boost@lists.boost.org>:
On Mon, Oct 30, 2017 at 12:47 AM, Andrzej Krzemienski via Boost <boost@lists.boost.org> 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;
On Mon, Oct 30, 2017 at 2:44 AM, Andrzej Krzemienski via Boost <boost@lists.boost.org> 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.
2017-10-30 11:03 GMT+01:00 Vinnie Falco via Boost <boost@lists.boost.org>:
On Mon, Oct 30, 2017 at 2:44 AM, Andrzej Krzemienski via Boost <boost@lists.boost.org> 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;
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.
On Mon, Oct 30, 2017 at 7:53 AM, Peter Dimov via Boost < boost@lists.boost.org> 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/1tpo3... .\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
2017-11-06 20:55 GMT+01:00 James E. King, III via Boost < boost@lists.boost.org>:
On Mon, Oct 30, 2017 at 7:53 AM, Peter Dimov via Boost < boost@lists.boost.org> 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;
participants (5)
-
Andrzej Krzemienski -
James E. King, III -
John Maddock -
Peter Dimov -
Vinnie Falco