Hah! It is in the public API, it's being passed around after the lock in the .cpp file. I don't think there's a way, is there?

I don't think there's a way, is there?

Creating a pcl::weak_ptr and then replacing with std in the alias would be one.

I have it locally, but it seems way too much for a single instance honestly, don't you think?

Here you can see what I had

Another option is to create a local type-alias, like used for all shared_ptr ClassName::Ptr complemented by ClassName::WeakPtr

Going down the rabbit hole of fixing the weak pointers leads to the app Point Cloud Editor. There, it becomes an exercise in futility as there's a circular dependency between the classes Cloud and Selection impeding progress if one wanted to have nice Cloud::Ptrs and Selection::Ptrs together with the Selection::WeakPtr. There only one SO suggestion atm but it's even more work and it doesn't really fit with the rest of the codebase.

I stand by the idea of leaving the toggle where it is currently for std::weak_ptrs. I'll push the non-compiling code so you can cringe with me. To continue here you really need to love the idea of this pcl::weak_ptr.

Some of the patch could be integrated as is, while other needs change (for the same reasons as circular dependency). Only public facing API needs to be type-aliased, and I don't think apps module fits that (but io does).

There shouldn't be a circular dependency. The reason is that the class which uses the type-alias is the one declaring it, not the class being used. If this were not the case, every class will need to come with it's own Ptr, WeakPtr declarations and impose them on downstream users. We don't care how the downstream users use our class, only that the internals of the class remain consistent.

#include <memory>
#include <boost/shared_ptr.hpp>
//---
class A;
class B;

class A{
using DataPtr = std::shared_ptr<B>;
DataPtr data;
};
class B {
using ControllerPtr = boost::shared_ptr<A>;
using ControllerWeakPtr = boost::shared_ptr<A>;
ControllerWeakPtr controller;
ControllerPtr fixController();
};

Only public facing API needs to be type-aliased, and I don't think apps module fits that (but io does).

You think I should keep the typedef confined to io?

the class which uses the type-alias is the one declaring it, not the class being used

Are you suggesting to remove these (in PointCloudEditor only) in favour of private typedefs with the full name CloudPtr, SelectionWeakPtr? I thought it would be inconsistent with the rest of the PRs tackling this. If we were sticking to using Ptr, ConstPtr and therefore WeakPtr, what would you include/forward declare in:

• ../selection.h:

• ../cloud.h:

in favour of private typedefs with the full name CloudPtr, SelectionWeakPtr?

Yeah, sort of, except replace with public type alias if the type is used in public API, else no type alias needed.

I thought it would be inconsistent with the rest of the PRs tackling this

PCL hasn't been perfect over the years, and as such there have been exceptions in the code. The best example would be use of Indices in pcl_base.h based on PointIndices.h.

IndicesPtr is declared outside the class and Ptr inside PointIndices, though have the same underlying type. Another type alias is created (PointIndicesPtr) in pcl_base but not preferred. However, if we were to look beyond the usage, at the usage, we see that:

1. only PointIndicesPtr is strictly needed, and available in most classes (I didn't number crunch this, but such aliases were present in all 50 classes that I saw)
2. IndicesPtr is a type-alias to prevent creating this new alias everywhere
3. PointIndices::Ptr is not beneficial in any sense, and is seldom used (X::Ptr usage is 3.5k compared to 10k usage of XPtr, aka like 1 and 2)

If we were sticking to using Ptr, ConstPtr and therefore WeakPtr, what would you include/forward declare in

That's the issue. This approach is ok in most cases, but fails when it causes circular references. On the other hand, using approach 1 and 2 (declared when needed and externally declared) don't cause such issues.

Notes on 4041300: is this how you intended it to be? If it is, I can finish up by removing all the using declarations on localTypes.h so that the whole app compiles with aliases in each class.

is this how you intended it to be?

Yes. Please note that you can exclude aliases for something not in public domain to reduce the work needed to be done.

I was doing it but realized there's no real reason to remove aliases in localTypes.h in this PR. This is currently the minimum to get this std::weak_ptr "nice" switch working. What are the required changes to get this merged after a rebase?

Hey @kunaltyagi any feedback here?

Guys, this PR spiraled out of control. The last boost::weak_ptr replacement will need to happen alongside the shared pointer alias switch.

/pcl/io/src/openni_camera/openni_driver.cpp: In member function ‘openni_wrapper::OpenNIDevice:
:Ptr openni_wrapper::OpenNIDriver::getDeviceByIndex(unsigned int) const’:                                                      
/pcl/io/src/openni_camera/openni_driver.cpp:291:17: error: could not convert ‘device’ from ‘st
d::shared_ptr<openni_wrapper::OpenNIDevice>’ to ‘openni_wrapper::OpenNIDevice::Ptr {aka boost::shared_ptr<openni_wrapper::OpenN
IDevice>}’                                                                                                                     
   return (device); 

This would have been enough

$ git diff
diff --git a/common/include/pcl/memory.h b/common/include/pcl/memory.h
index 4037c5ace..009f7ee91 100644
--- a/common/include/pcl/memory.h
+++ b/common/include/pcl/memory.h
@@ -47,6 +47,7 @@
 
 #include <boost/make_shared.hpp>  // for boost::allocate_shared, boost::make_shared
 #include <boost/smart_ptr/shared_ptr.hpp>  // for boost::shared_ptr
+#include <boost/smart_ptr/weak_ptr.hpp>  // for boost::weak_ptr
 
 #include <Eigen/Core>  // for EIGEN_MAKE_ALIGNED_OPERATOR_NEW
 
@@ -80,6 +81,9 @@ namespace pcl
 template <typename T>
 using shared_ptr = boost::shared_ptr<T>;
 
+
+using boost::weak_ptr;
+
 #ifdef DOXYGEN_ONLY
 
 /**
diff --git a/io/include/pcl/io/openni_camera/openni_driver.h b/io/include/pcl/io/openni_camera/openni_driver.h
index 6ece49406..9317a7853 100644
--- a/io/include/pcl/io/openni_camera/openni_driver.h
+++ b/io/include/pcl/io/openni_camera/openni_driver.h
@@ -43,6 +43,7 @@
 #include "openni_exception.h"
 #include "openni_device.h"
 #include <pcl/io/boost.h>
+#include <pcl/memory.h>
 #include <pcl/pcl_macros.h>
 
 #include <map>
@@ -217,7 +218,7 @@ namespace openni_wrapper
       std::shared_ptr<xn::NodeInfo> image_node;
       std::shared_ptr<xn::NodeInfo> depth_node;
       std::shared_ptr<xn::NodeInfo> ir_node;
-      boost::weak_ptr<OpenNIDevice> device;
+      pcl::weak_ptr<OpenNIDevice> device;
     } ;
 
     OpenNIDriver ();

Why is this PR touching ~1800 lines to address this problem?

Most of those touched lines are formatting/spaces, so out of scope, I guess. The rest is to have all weak_ptr have the same singular "switch" inside memory.h instead of needing separate toggles. This is useless if the only plan is to deprecate the switch itself (pcl::weak_ptr) in favour of using std::weak_ptr directly. It is instead useful if at any time in the future a need arises to change the pointer's implementation.

I'm pretty sure the error shows how all the edits in point_cloud_editor are mandatory:

/__w/1/s/apps/point_cloud_editor/src/deleteCommand.cpp:69:44: error: no matching function for call to 'Cloud::setSelection(SelectionPtr&)'
   cloud_ptr_ -> setSelection(selection_ptr_);
                                            ^
In file included from /__w/1/s/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/copyBuffer.h:44:0,
                 from /__w/1/s/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/deleteCommand.h:45,
                 from /__w/1/s/apps/point_cloud_editor/src/deleteCommand.cpp:41:
/__w/1/s/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h:213:5: note: candidate: void Cloud::setSelection(const SelectionPtr&)
     setSelection (const SelectionPtr& selection_ptr);
     ^
/__w/1/s/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h:213:5: note:   no known conversion for argument 1 from 'SelectionPtr {aka std::shared_ptr<Selection>}' to 'const SelectionPtr& {aka const boost::shared_ptr<Selection>&}'
apps/point_cloud_editor/CMakeFiles/pcl_point_cloud_editor.dir/build.make:217: recipe for target 'apps/point_cloud_editor/CMakeFiles/pcl_point_cloud_editor.dir/src/deleteCommand.cpp.o' failed
make[2]: *** [apps/point_cloud_editor/CMakeFiles/pcl_point_cloud_editor.dir/src/deleteCommand.cpp.o] Error 1
make[1]: *** [apps/point_cloud_editor/CMakeFiles/pcl_point_cloud_editor.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

and therefore #3949 would become only a change in the realsense file. Should that be the case?

This error is because global SelectionPtr (in localTypes.h) is being masked by the class's internal SelectionPtr. We can resolve it by

• preventing name clash (optional, but preferred) + switch the global SelectionPtr to pcl::shared_ptr
• removing the global type and using class local types only (your suggestion, except you're keeping the global type)
• modify the global type and remove class local types

I've no idea which is better for apps module.

diff --git a/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h b/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h
index fc66674ec..3d1fd80c9 100644
--- a/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h
+++ b/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h
@@ -78,11 +78,8 @@
 class Cloud : public Statistics
 {
   public:
-    /// The type for shared pointer pointing to a selection buffer
-    using SelectionPtr = pcl::shared_ptr<Selection>;
-
     /// The type for weak pointer pointing to a selection buffer
-    using SelectionWeakPtr = pcl::weak_ptr<Selection>;
+    using SelectionWeakPtr = std::weak_ptr<Selection>;
 
     /// @brief Default Constructor
     Cloud ();

This fixed it for me.

The point was having a single switch. Changing the scope of the PR means going directly back to #3753 (comment) to me, there would be no point in being more verbose than those few line changes.

And, reiterating, I believe all the changes in the now separate PR were due to this CI error we see now. I'll merge that bit in this PR and leave out only the API changes in that PR which are likely only the ones in RealSense scope.

The file localTypes.h has confusing entries now. The classes are overriding the SelectionPtr so it might make sense to remove the one unneeded local type to avoid confusion.

https://github.com/PointCloudLibrary/pcl/blob/master/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/localTypes.h#L77

I absolutely know it, that's why the commit regarding point_cloud_editor is named as it is. Also please refer to #3753 (comment) for why I'd much rather create an issue and maybe work on it another time. It's a VERY long ordeal to remove that file's typedefs.

EDIT: the reason is I tried and it's a long ordeal, there are lots of files using that header.

Issue opened