Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix small_vector noexcept specification #114

Closed

Conversation

@Lastique
Copy link
Member

commented Apr 13, 2019

  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

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

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

@igaztanaga

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Reviewing the patch, some comments:

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

  • I'm unsure about marking some constructors conditionally noexcept (like the variadic constructor). It complicates code a lot and I don't think performance would improve. Not sure also if the variadic constructor is needed at all, I think it is not used, I will review the use cases.

@Lastique

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

Cpp17Allocator requirements require noexcept for allocator move and copy constructors. This means that looking for dtl::is_nothrow_move_constructible<allocator_type>::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.

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

@Lastique Lastique force-pushed the Lastique:fix_small_vector_noexcept branch from 048a99d to 46fbee6 Apr 26, 2019

@Lastique

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

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

@igaztanaga

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

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.

@Lastique Lastique deleted the Lastique:fix_small_vector_noexcept branch Apr 26, 2019

Lastique added a commit to Lastique/container that referenced this pull request May 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.