From new to pcl::make_shared using clang-tidy

[aPonza]

Takes all the relevant commits from #3146 and rebases them on the branch from #3163.

[SergioRAgostinho]

I just checked the CI errors and some of the replaces got botched up, e.g.:

indices.reset = pcl::make_shared<pcl::PointIndices> ();

[SergioRAgostinho]

I've aborted the review of this PR early on because it has some recurring issues. After you rebase things, I would recommend going through the diff and reviewing the changes you made in more detail. I don't thing clang-tidy will be enough here.

[SergioRAgostinho]

The line above this one should also be replaced no?

[aPonza]

PointInT is a template parameter, I didn't test for that and apparently the check is failing with those. As is, I also have a failing test for when there is typedef boost::shared_ptr<Derived> Ptr;

[SergioRAgostinho]

Same comment as before.

[SergioRAgostinho]

You can use std::make_shared here.

[taketwo]

Is replacement of every reset with make_shared our goal? Personally, I see reset-based code as more maintainable in a sense that it does not need to know whether the pointer is std or boost.

[SergioRAgostinho]

However, you can't use pcl::make_shared here. Notice this object has a custom allocator but won't have the necessary trait in order for dispatching to work properly.

[taketwo]

In this case the custom allocator is for contained objects, not the vector itself. Therefore, default allocator is suitable for the vector. However, I'd invoke the same comment as above here, in my opinion no need to replace reset with make_shared.

[SergioRAgostinho]

Same comment here for line 602 and 603.

[SergioRAgostinho]

I'm moving this answer to "global scope" because it is affects the entire PR.

Is replacement of every reset with make_shared our goal? Personally, I see reset-based code as more maintainable in a sense that it does not need to know whether the pointer is std or boost.

I thought that was the goal: having the one time allocation and prevent mem leaks in those very unlikely corner cases.

I also understand your maintainability point but isn't it just a temporary thing? I.e., if we halt this PR until the boost -> std pointer migration is complete, then it no longer matters, because there should only exist std pointers in the code base.

That being said, this PR carries a lot of changes. I would really prefer to have this split by module so that we can progressively merge things and evaluate the results in the following days.

[taketwo]

I also understand your maintainability point but isn't it just a temporary thing?

This is true. However I should admit that as part of migration effort I'm generally trying to convert make_shared instances to reset for the reason I cited above. My idea for the current phase of migration is to get rid of all "raw" (i.e. with explicit boost) usages of smart pointers. Now that pcl::make_shared is available, should I rather convert boost::make_shared to it?

I would really prefer to have this split by module so that we can progressively merge things and evaluate the results in the following days.

👍

[SergioRAgostinho]

Now that pcl::make_shared is available, should I rather convert boost::make_shared to it?

I would say yes, but I feel I don't have a good answer for this. I suspect this might generate some compilation issues during the transition period, in case some modules are already transitioned to std shared/unique pointers and others not. I still expect the compiler to simply fail, so we can always adjust whatever needed accordingly.

If there was no penalty in the time invested, I would really finish the transition as you're doing right now and only then start applying pcl::make_shared whenever applicable.

[kunaltyagi]

reset vs make_shared

On an somewhat related note, make_shared increases compile time, RAM usage at compile time and binary size as compared to reset. However, it frequently results in speed ups because it performs one allocation and has the control block and the pointer stored together.

Clang and GCC both show a 10% performance difference.

This only matters in the hot path, so if

• the allocations aren't happening in a loop
• the pointers live a long life (less updates to the control block)
• the number of types is a lot
  Then, the performance diff would be negligible, and reset would outshine make_shared.

Maybe this helps you in making a good tradeoff

[aPonza]

if we halt this PR until the boost -> std pointer migration is complete, then it no longer matters, because there should only exist std pointers in the code base.

It would still carry the benefit of maintainers/contributors (same as end-users) not having to know which objects require custom allocation, and not having as many raw news, but sure, its value would also be in automatically "flipping the switch" on many boost->std conversions.

I suspect this might generate some compilation issues during the transition period, in case some modules are already transitioned to std shared/unique pointers and others not.

If this graph is accurate I'm thinking it might work for some groupings of modules, potentially making it ok for review as well. E.g. keypoints, registration, search, surface, tracking, visualization, should all go down easily, then continue from there? Am I missing something?

BTW the code for the clang-tidy check is 99% the same as the one for modernize-make-shared (check and test) if it helps understand why it fails here and there.

[SergioRAgostinho]

@kunaltyagi thank you for the extra insight. In my experience, there should not exist shared pointers created being inside hot loops. Hot loops in PCL only tend happen inside computation classes. These don't usually allocate more than a couple of shared pointers if any.

@aPonza I'm not sure I conveyed the issue properly or I am simply not understanding your followup comments. pcl::make_shared is defined in common and currently uses of boost::make_/allocate_shared. If we convert all (appropriate) instances of reset into pcl::make_shared, then we are effectively restricting ourselves to boost pointers. This means that if we want to do a progressive transition by module, we cannot use the current pcl::make_shared as it is merged.

If we already migrate pcl::make_shared to std, then have at least the option to only start using it in modules which already underwent the boost -> std transition.

[aPonza]

No I'm the one who hadn't understood. I was thinking (e.g. in keypoints) about:

• converting all the appropriate instances of reset/new to pcl::make_shared in there is compilable;
• then switching all remaining instances to std:: and pcl::make_shared itself to std:: (i.e. moving keypoints to std::) keeps everything compilable because there should be no modules depending on that module (according to the graph). Here's the problem: features (which depends on keypoints) probably uses boost:: things on keypoints stuff, which is now std:: and there compilation is broken. I think that was what I had not realized. So you're right, now it's crystal clear.