boostorg / container

STL-like containers from Boost
http://www.boost.org/libs/container/
Boost Software License 1.0
100 stars 116 forks source link

Fix small_vector noexcept specification #114

Closed Lastique closed 5 years ago

Lastique commented 5 years ago
  1. Move small_vector_base forward-declaration to container_fwd.hpp. This is useful if the user only wants to use small_vector_base in its interface headers.
  2. Fix small_vector default constructor noexcept specification.

The default constructor used to incorrectly apply is_nothrow_default_constructible trait to the template argument of the small_vector class template, which is void by default. The fix is to apply the trait to the actual allocator type.

Next is the fix for noexcept specifications of the small_vector_allocator class members, including the default constructor, which was previously not declared and a non-noexcept initializing constructor was used in its stead. The fix is to explicitly specify the default constructor with a proper noexcept specification. While at it, other constructors and assignment operators were also fixed with proper noexcept specifications.

As a result, small_vector default constructor noexcept specification now properly reflects whether the default constructor of the allocator is noexcept. This is useful if user's class has small_vector members and attempts to request a noexcept defaulted default constructor.

igaztanaga commented 5 years ago

Thanks for the patch. Commit 5ec7aa7 cherry-picked to develop. Investigating why continuous integration fails with the noexcept specificcation.

igaztanaga commented 5 years ago

Reviewing the patch, some comments:

Lastique commented 5 years ago

Cpp17Allocator requirements require noexcept for allocator move and copy constructors. This means that looking for dtl::is_nothrow_move_constructible::value and similar is redundant.

I didn't know allocator copy and move are required to be noexcept. Not sure if every allocator in the wild follows this requirement, though, so I tend to prefer the traits. But I can change to BOOST_NOEXCEPT no problem, if you want me to.

I'm unsure about marking some constructors conditionally noexcept (like the variadic constructor).

I'd like the constructors to be as transparent as possible.

Also, I seem to remember that at least some compilers prefer the templated constructors to non-templated ones like copy constructors in some cases. I think, it was when you construct a copy from a non-const allocator variable, but maybe there were other cases. For this reason, the templated constructor should have the same effect, including noexcept specification, as the non-templated one.

Lastique commented 5 years ago

I fixed a typo in the macro and rebased to the current develop. This should fix C++03 tests.

igaztanaga commented 5 years ago

Thanks. All other containers assume allocator copy/move don't throw, so I've modified your patch to inconditionally make some operations noexcept. The variadic constructor was not needed at all so I've removed it, smalLvector_allocator is an internal class so no one should notice this.