search

boostorg / container

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

polymorphic_allocator(memory_resource*) non-standard extension causes headache #161

Closed vinniefalco closed 3 years ago

vinniefalco commented 3 years ago

This non-standard extension: https://github.com/boostorg/container/blob/5a52472cd00994bf1752e92c1c178a5a822a36cb/include/boost/container/pmr/polymorphic_allocator.hpp#L56

Requires Boost.JSON users to link to Boost.Container even if they are not using the default resource (which they likely will never be). It also allows programs to rely on non-standard behavior, making them more difficult to port to C++17. I believe this non-standard extension should be removed. Construction of polymorphic_allocator with a null memory resource should throw something like logic_error or invalid_parameter.

igaztanaga commented 3 years ago

Throwing an exception would be non-conforming, so the best option here is to remove the non-standard extension and add a simple assert in case a null pointer is passed.

igaztanaga commented 3 years ago

Extension removed, thanks for the report.

vinniefalco commented 3 years ago

Glad to help. Note that the upcoming Boost.JSON scheduled for formal review September 14-24 relies heavily on Boost.Container's memory_resource and polymorphic_allocator as building blocks for the discriminated union containers.

What happens if users are depending on this extension? I thought throwing an exception (and causing their app to terminate) was preferable to getting possibly undefined behavior.

igaztanaga commented 3 years ago

The function was not correctly specified as it says (https://www.boost.org/doc/libs/1_74_0/doc/html/boost/container/pmr/polymorphic_allocator.html#idm45720956936592-bb):

Requires: r is non-null.

and the note:

Non-standard extension: if r is null m_resource is set to get_default_resource().

I think the removal will not cause much pain, as it was not clear if null was or not valid.

vinniefalco commented 3 years ago

Oh! Great! I didn't actually notice "Requires: r is non-null." when I looked at it :)