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

Replace auto_ptr with scoped_ptr #2037

Merged

Conversation

moriarty
Copy link
Contributor

C++11 deprecated auto_ptr and c++17 removes it.

It looks like this is just a straight forward replacement... it's being used in the pimpl pattern.

C++11 deprecated auto_ptr and c++17 removes it.
@moriarty
Copy link
Contributor Author

NOTE:

Locally, I have some issues with the unit tests, which I suspect are due to my setup...
If Travis fails on the same tests, I will investigate further.

Total Test time (real) = 202.82 sec

The following tests FAILED:
	 13 - common_eigen (Failed)
	 23 - feature_rift_estimation (Failed)
	 25 - feature_cppf_estimation (Failed)
	 27 - feature_pfh_estimation (Failed)
	 48 - filters_sampling (Failed)
	100 - test_non_linear (Failed)
Errors while running CTest

@moriarty
Copy link
Contributor Author

Ignore my comment above. Instead of waiting for Travis, I checked out master and compiled and ran the tests... and had the same result.

Total Test time (real) = 173.14 sec

The following tests FAILED:
	 13 - common_eigen (Failed)
	 23 - feature_rift_estimation (Failed)
	 25 - feature_cppf_estimation (Failed)
	 27 - feature_pfh_estimation (Failed)
	 48 - filters_sampling (Failed)
	100 - test_non_linear (Failed)
Errors while running CTest

So they are failing due to something else in my Setup.

@moriarty moriarty closed this Oct 24, 2017
@moriarty
Copy link
Contributor Author

Closing this, not only because clang failed in Travis, directly due to this change...

But because after reading the discussion in #2035 I now see that PCL supports old compilers which don't have C++11

@moriarty
Copy link
Contributor Author

I can not reproduce the Travis error with clang locally... but it's obviously because of the lack of C++11.

alex@localhost:~/workspace/pcl-trunk> clang --version
clang version 4.0.1 (tags/RELEASE_401/final 305264)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
alex@localhost:~/workspace/pcl-trunk> gcc --version
gcc (SUSE Linux) 7.2.1 20171005 [gcc-7-branch revision 253439]
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@taketwo
Copy link
Member

taketwo commented Oct 25, 2017

Some of the tests are known to fail in environments different from what we have on Travis. Often the problems are related to fp precision.

Regarding this PR, perhaps you can replace auto_ptr with a Boost alternative?

@moriarty
Copy link
Contributor Author

moriarty commented Oct 25, 2017

👍 yep I can use a Boost alternative.

The previous commit, replaced the removed auto_ptr with C++11
unique_ptr.
PCL supports older C++ versions, so Boost.Move unique_ptr is needed.
@moriarty moriarty reopened this Oct 25, 2017
@moriarty
Copy link
Contributor Author

@taketwo I didn't squash my commit yet, I don't have the older version of clang to test against, so I reopened the PR to use Travis.

@moriarty
Copy link
Contributor Author

👎 boost/move/unique_ptr was added with Boost 1.57 and you're testing with 1.55, and your pcl_find_boost.cmake goes all the way back to 1.40.

I'll close this again.
It will need to be fixed for C++17 where auto_ptr is removed, but until then it's just a warning.

Do you have a label or big issue for tracking large breaking changes?

@moriarty moriarty closed this Oct 25, 2017
@taketwo
Copy link
Member

taketwo commented Nov 2, 2017

Sorry for the delay, was on vacation.

The "traditional" Boost alternative for unique_ptr is called scoped_ptr and is available since at least 1.40.

Regarding a label breaking changes, we don't have one, but it's a good idea to have. What do you think @SergioRAgostinho?

@SergioRAgostinho
Copy link
Member

For visually identifying the PRs? Sure.

@taketwo
Copy link
Member

taketwo commented Nov 2, 2017

Also will be helpful when putting together the changelist.

@moriarty
Copy link
Contributor Author

moriarty commented Nov 2, 2017

I’m away, I will be able to send another PR next week.

@rakshith95
Copy link

In file included from /home/rakshith/pcl/visualization/tools/image_grabber_saver.cpp:43:0:
/home/rakshith/pcl/visualization/include/pcl/visualization/cloud_viewer.h:202:14: warning: ‘template class std::auto_ptr’ is deprecated [-Wdeprecated-declarations]
std::auto_ptr<CloudViewer_impl> impl_;
^~~~~~~~
In file included from /usr/include/c++/7/memory:80:0,
from /usr/include/boost/config/no_tr1/memory.hpp:21,
from /usr/include/boost/smart_ptr/shared_ptr.hpp:23,
from /usr/include/boost/shared_ptr.hpp:17,
from /usr/include/boost/date_time/time_clock.hpp:17,
from /usr/include/boost/thread/thread_time.hpp:9,
from /usr/include/boost/thread/lock_types.hpp:18,
from /usr/include/boost/thread/pthread/mutex.hpp:16,
from /usr/include/boost/thread/mutex.hpp:16,
from /home/rakshith/pcl/io/include/pcl/io/boost.h:49,
from /home/rakshith/pcl/io/include/pcl/io/grabber.h:48,
from /home/rakshith/pcl/io/include/pcl/io/image_grabber.h:46,
from /home/rakshith/pcl/visualization/tools/image_grabber_saver.cpp:39:
/usr/include/c++/7/bits/unique_ptr.h:51:28: note: declared here
template class auto_ptr;

I'm getting a lot of this type of warnings during build process. What kind of problems can it cause? I noticed this while re building because I was having segmentation faults for certain programs, and now I'm wondering if it could be because of this.

@SergioRAgostinho
Copy link
Member

auto_ptr is a type which was redesigned and replaced by unique_ptr in c++11 and it means that at some point, in some c++ standard version, it will be deleted and not defined anymore. This is not the cause of your segfaults.

@taketwo
Copy link
Member

taketwo commented Nov 14, 2017

Actually it's already removed from C++17

auto_ptr is removed with C++17.

C++11 std::unique_ptr and boost::move::unique_ptr are not old enough for all platforms supported by PCL.
@moriarty moriarty reopened this Nov 15, 2017
@moriarty
Copy link
Contributor Author

Sorry I forgot about this and could have just fixed it from the GitHub web interface (which is what I just did)... I will squash the commits if Travis passes.

@SergioRAgostinho
Copy link
Member

We can do the squasht for you, no worries.

@moriarty
Copy link
Contributor Author

The job exceeded the maximum time limit for jobs, and has been terminated. :(

@SergioRAgostinho SergioRAgostinho merged commit 651c06a into PointCloudLibrary:master Nov 15, 2017
UnaNancyOwen pushed a commit to UnaNancyOwen/pcl that referenced this pull request Nov 24, 2017
@moriarty moriarty deleted the replace-autoptr-w-uniqueptr branch May 22, 2018 15:25
@taketwo taketwo changed the title [c++] replace auto_ptr with unique_ptr Replace auto_ptr with scoped_ptr Sep 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants