boostorg / ptr_container

Boost.org ptr_container module
http://boost.org/libs/ptr_container
Boost Software License 1.0
13 stars 41 forks source link

Conditionally provide interfaces based on deprecated/removed std::aut… #8

Closed DanielaE closed 6 years ago

DanielaE commented 7 years ago

…o_ptr and/or std::unique_ptr, and replace C++98 function adapters by inline typedefs.

Signed-off-by: Daniela Engert dani@ngrt.de

pdimov commented 6 years ago

I considered merging that but I don't like the introduction of BOOST_PTR_CONTAINER_NO_AUTO_PTR and have no time now to remove it. :-)

DanielaE commented 6 years ago

So, my question is:

My idea was to give users both all technically possible interfaces (i.e. stay as compatible as possible) and an opt-out-of-auto_ptr option, too.

eldiener commented 6 years ago

The opt-out-of-auto_ptr option is totally unnecessary. If BOOST_NO_CXX11_SMART_PTR is defined, leave the code as is, else use std::unique_ptr in place of std::auto_ptr. That is all you have to do.

pdimov commented 6 years ago

Second bullet, @DanielaE. What you have, just with BOOST_CONTAINER_NO_AUTO_PTR removed, and BOOST_NO_AUTO_PTR used directly in its place.

DanielaE commented 6 years ago

There it is, changes applied as advised. I'm sorry for the delay, but attending the Meeting C++ conference, a long business trip, life - you know 😄

pdimov commented 6 years ago

No problem, we're all volunteers here. :-)

pdimov commented 6 years ago

Are there any other libraries for which auto_ptr pull requests are sitting unmerged?

eldiener commented 6 years ago

You should not be testing be testing BOOST_NO_AUTO_PTR. You should only be testing BOOST_NO_CXX11_SMART_PTR. When BOOST_NO_CXX11_SMART_PTR is not defined, use unique_ptr, otherwise use auto_ptr. It really is that simple. If both BOOST_NO_AUTO_PTR and BOOST_NO_CXX11_SMART_PTR is defined you have a broken compiler, and nothing will work so you just do not waste your time with such a possibility.

Please explain why you think you have to test BOOST_NO_AUTO_PTR ?

pdimov commented 6 years ago

If we just remove the auto_ptr overloads when BOOST_NO_CXX11_SMART_PTR isn't defined, won't this break existing code using auto_ptr under C++11?

eldiener commented 6 years ago

My bad. I did not look to see that auto_ptr is part of the public/protected interface. In that case it should be changed to unique_ptr only if BOOST_NO_AUTO_PTR is defined. My apologies. I had changed a number of libraries from auto_ptr to unique_ptr, but none of them used auto_ptr except for private or detail functionality.

eldiener commented 6 years ago

I should have said, for the public/protected interfaces, change auto_ptr to unique_ptr only if BOOST_NO_AUTO_PTR is defined and BOOST_NO_CXX11_SMART_PTR is not defined, else drop the auto_ptr interface entirely; although I can not imagine any compiler for which neither auto_ptr nor unique_ptr is supported for a given C++ standard compiler level.

DanielaE commented 6 years ago

@pdimov:

Are there any other libraries for which auto_ptr pull requests are sitting unmerged?

Yes, there are. I have a list of open PRs which I submitted months ago. I will re-check it when I am back from work.

Update: I have open PRs regarding std::auto_ptr with asio, assign, and statechart.

DanielaE commented 6 years ago

@pdimov I've updated the tests once again ...

pdimov commented 6 years ago

This looks good to me.

I'm seeing one odd error on msvc-14.1/cxxstd-17:

compile-c-c++ ..\..\bin.v2\libs\ptr_container\test\ptr_map_adapter.test\msvc-14.1\debug\cxxstd-17-iso\threadapi-win32\threading-multi\ptr_map_adapter.obj
ptr_map_adapter.cpp
c:\program files (x86)\windows kits\10\include\10.0.16299.0\shared\rpcndr.h(192): error C2872: 'byte': ambiguous symbol
c:\program files (x86)\windows kits\10\include\10.0.16299.0\shared\rpcndr.h(191): note: could be 'unsigned char byte'
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\cstddef(15): note: or       'std::byte'

This doesn't look like our problem, but it might be possible to work around it (or it could be my config.)

pdimov commented 6 years ago

Fixed it, needed to change <boost/test/included/unit_test.hpp> to <boost/test/unit_test.hpp>. I'm ready to merge if Edward doesn't object.

eldiener commented 6 years ago

I do not object since I messed up originally in not understanding that the PR dealt with non-private interfaces. The code looks good and I am sorry I was such an annoyance.

pdimov commented 6 years ago

https://github.com/boostorg/asio/pull/51 seems no longer relevant.

pdimov commented 6 years ago

Nobody has write access to Statechart. :-)

pdimov commented 6 years ago

Assign's Travis fails, will need to figure out why.

DanielaE commented 6 years ago

Great, Boost.ASIO got some love by Chris 👍 So it seems like Boost.Locale is the last lib standing. AFAIK Artyom objects changes on behalf of auto_ptr.