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

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

Merged

Conversation

DanielaE
Copy link
Contributor

…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
Copy link
Member

pdimov commented Nov 5, 2017

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
Copy link
Contributor Author

DanielaE commented Nov 7, 2017

So, my question is:

  • don't you like the name?
  • do you want the inclusion of the auto_ptr interface to be dependent on BOOST_NO_AUTO_PTR only?
  • do you want the inclusion of the auto_ptr interface to be dependent on BOOST_NO_CXX11_SMART_PTR only?

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
Copy link
Contributor

eldiener commented Nov 7, 2017

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
Copy link
Member

pdimov commented Nov 7, 2017

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 DanielaE force-pushed the feature/replace-deprecated-c++98-stuff branch from 979a101 to ac21428 Compare November 19, 2017 15:47
@DanielaE
Copy link
Contributor Author

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
Copy link
Member

pdimov commented Nov 19, 2017

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

std::auto_ptr<T> ap( new T );
c.insert( ap );
#else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're providing the unique_ptr overloads when BOOST_NO_CXX11_SMART_PTR is not defined, we should probably test them under the same condition, instead of only when BOOST_NO_AUTO_PTR is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but that doesn't work out in all cases: in return values you only have either/or exclusively. Have a peek at line 139:

#ifndef BOOST_NO_AUTO_PTR
    std::auto_ptr<C> ap2        = c.release();
#else
    std::unique_ptr<C> up2      = c.release();
#endif

Sigh - this testing stuff will become tedious then...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. Perhaps just do it for the cases where it can be done easily, and leave the tedious ones as is?

@pdimov
Copy link
Member

pdimov commented Nov 19, 2017

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

@eldiener
Copy link
Contributor

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
Copy link
Member

pdimov commented Nov 19, 2017

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

DanielaE commented Nov 20, 2017

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

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

Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE DanielaE force-pushed the feature/replace-deprecated-c++98-stuff branch from ac21428 to b805b3c Compare November 20, 2017 18:25
@DanielaE
Copy link
Contributor Author

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

@pdimov
Copy link
Member

pdimov commented Nov 20, 2017

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
Copy link
Member

pdimov commented Nov 20, 2017

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
Copy link
Contributor

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 pdimov merged commit b805b3c into boostorg:develop Nov 20, 2017
@pdimov
Copy link
Member

pdimov commented Nov 20, 2017

boostorg/asio#51 seems no longer relevant.

@pdimov
Copy link
Member

pdimov commented Nov 20, 2017

Nobody has write access to Statechart. :-)

@pdimov
Copy link
Member

pdimov commented Nov 20, 2017

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

@DanielaE
Copy link
Contributor Author

DanielaE commented Nov 21, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants