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

Use pcl::make_shared instead of new for boost::shared_ptr #3146

Closed
wants to merge 27 commits into from

Conversation

aPonza
Copy link
Contributor

@aPonza aPonza commented Jun 12, 2019

I added the code from Sergio in what seemed to be a logical place, wrote the clang-tidy check and let it do its magic. I'm fixing a couple of mistakes it made, but after I'm done compiling it seems ready to commit: should I separate the commits on a per-folder basis to make the review job easier? I'm sitting on 123 files changed, 587 insertions(+), 346 deletions(-) (still finishing the fixup).

By the way, I left boost:: functions inside pcl::make_shared to keep the compatibility until the rest is switched to std:: so that it should be easier to flip the switch with only small changes in the implementation header. Do let me know if you'd rather have standard library pointers already.

EDIT: while fixing I'm noticing it missed a lot of them, despite the tests passing in the checker... not sure why, it's all cases of typedeffed ::Ptr stuff.

@SergioRAgostinho SergioRAgostinho added the changelog: enhancement Meta-information for changelog generation label Jun 12, 2019
Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

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

This will have a direct effect on the work @taketwo is conducting, migrating things from <memory> from boost to std.

I don't think there is much value in splitting this into .h and .hpp in this case. So I would propose to keep everything in .h.

Be critic about my review. If you think the are better alternatives to my suggestions be sure to bring them up.

common/include/pcl/impl/make_shared.hpp Outdated Show resolved Hide resolved
common/include/pcl/impl/make_shared.hpp Outdated Show resolved Hide resolved
common/include/pcl/impl/make_shared.hpp Outdated Show resolved Hide resolved
template<typename U> static int32_t test(...);
public:
static constexpr bool value = (sizeof(test<T>(0)) == sizeof(char));
};
Copy link
Member

Choose a reason for hiding this comment

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

This class should likely be placed in https://github.com/PointCloudLibrary/pcl/blob/master/common/include/pcl/point_traits.h . My issue with that is that the file is named point_traits. @taketwo, proposal to rename to type_traits.

Copy link
Member

Choose a reason for hiding this comment

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

I would also propose to make the size difference more explicit by using a int8_t instead of char.

common/include/pcl/impl/make_shared.hpp Outdated Show resolved Hide resolved
common/include/pcl/impl/make_shared.hpp Outdated Show resolved Hide resolved
common/include/pcl/make_shared.h Outdated Show resolved Hide resolved
@SergioRAgostinho
Copy link
Member

I'm fixing a couple of mistakes it made, but after I'm done compiling it seems ready to commit: should I separate the commits on a per-folder basis to make the review job easier? I'm sitting on 123 files changed, 587 insertions(+), 346 deletions(-) (still finishing the fixup).

I prefer to do it that way, but a three-way split mind end up being perfectly fines as well. If there are slightly more complicated cases we weed them out for separate PRs.

I forgot to mention. We need doctring for the methods, to explain their purpose.

Andrea Ponza added 21 commits June 12, 2019 16:26
@SergioRAgostinho
Copy link
Member

I just skimmed through the followup commits and already noticed some problems.

  1. You did not find-replace EIGEN_MAKE_ALIGNED_OPERATOR_NEW for PCL_MAKE_ALIGNED_OPERATOR_NEW.
  2. Using #include "pcl/make_shared.h" instead of #include <pcl/make_shared.h>

Focus this PR only on creating make_shared and keep the other commits for follow-up PRs. The rest of the maintainers have not expressed their opinions on adopting this and that discussion needs to happen.

@aPonza
Copy link
Contributor Author

aPonza commented Jun 13, 2019

Yep, that's why it was still a draft, plus I needed some "early" feedback. I also noticed the double quotes, I'll fix it asap. Right now I finished with the compilation errors, fixed your first requests, and figured I was missing a test for the typedeffed boost::shared_ptr <Foo> Ptr which causes Foo::Ptr foo (new Foo); to not get fixed.

I'll retest compilation, fix point 2 and probably mark this as ready.

@aPonza aPonza marked this pull request as ready for review June 13, 2019 10:26
@aPonza
Copy link
Contributor Author

aPonza commented Jun 14, 2019

Focus this PR only on creating make_shared and keep the other commits for follow-up PRs. The rest of the maintainers have not expressed their opinions on adopting this and that discussion needs to happen.

I have a separate branch with the cherry-picked commits, to open a separate PR, and it's probably possible to edit this one/open a new one later with the clang-tidy stuff, correct?

@SergioRAgostinho
Copy link
Member

Yes. We can keep this one as the working draft and fork clean PRs with specific goals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants