Skip to content

Add weak_ptr typedefs to MOVEIT_DECLARE_PTR#206

Closed
de-vri-es wants to merge 2 commits intomoveit:kinetic-develfrom
fizyr-forks:weak_ptr_typedef
Closed

Add weak_ptr typedefs to MOVEIT_DECLARE_PTR#206
de-vri-es wants to merge 2 commits intomoveit:kinetic-develfrom
fizyr-forks:weak_ptr_typedef

Conversation

@de-vri-es
Copy link
Copy Markdown
Contributor

@de-vri-es de-vri-es commented Sep 8, 2016

Note: I'm not really sure if this PR is a good solution as it is. Opinions welcome.

This PR adds typedefs for weak_ptr to MOVEIT_DELCARE_PTR. Also discussed in #48, we want to tell people to use the shared_ptr typedefs, but that doesn't currently leave them a way to use the matching weak_ptr. This PR would solve that.

On the other hand, use of weak_ptr is much more rare. One use is to avoid shared_ptr cycles. Such cycles can only be made by embedding the pointers inside the pointed-to type(s), and users can't really do that. Of course, there are undoubtedly other uses for weak_ptrs that I'm not thinking of right now.

So, do we want to add a ton of exposed typedefs for probably a small if not zero number of niche use cases?

Slightly off-topic: All this macro hackery made me think a bit. A macro-free C++11 solution would be this in a public header somewhere:

template<typename T> using SharedPtr = boost::shared_ptr<T>;
template<typename T> using WeakPtr   = boost::weak_ptr<T>;

Or much less nice pre C++11:

template<typename T> struct SharedPtr { typedef boost::shared_ptr<T> type; };
template<typename T> struct WeakPtr   { typedef boost::weak_ptr<T>   type; };

Or even:

template<typename T>
struct Ptr {
  typedef boost::shared_ptr<T>       Shared;
  typedef boost::shared_ptr<const T> SharedConst;
  typedef boost::weak_ptr<T>         Weak;
  typedef boost::weak_ptr<const T>   WeakConst;
};

Or some variation on this theme. We could even use the not-so-pretty option just for weak_ptrs to avoid adding a ton of public typedefs.

@de-vri-es de-vri-es mentioned this pull request Sep 8, 2016
Closed
@davetcoleman
Copy link
Copy Markdown
Member

davetcoleman commented Sep 8, 2016

do we want to add a ton of exposed typedefs for probably a small if not zero number of niche use cases?

Is moveit_core/collision_detection/src/world_diff.cpp the only use case for weak_ptrs throughout all of moveit? If so, it doesn't seem adding this new macro is that necessary. The only reason we began this whole path of adding macros for shared_ptrs is so that we can minimize potential merge conflicts between different branches once we switch to C++11. This doesn't really help that effort unless we expect a lot of changes to these two files (world_diff.cpp/.h)

I don't think we should get too complex with these macros. Still, this PR seems ok to me.

@de-vri-es
Copy link
Copy Markdown
Contributor Author

de-vri-es commented Sep 8, 2016

Is moveit_core/collision_detection/src/world_diff.cpp the only use case for weak_ptrs throughout all of moveit?

Yes. All other usage of weak_ptr was for shapes::Shape which is not strictly speaking a moveit type.

We can't be sure that users don't need weak_ptrs for moveit types though. But I suppose they could even do something similar to this if it actually comes up:
https://github.com/ros-planning/moveit/blob/kinetic-devel/moveit_planners/ompl/ompl_interface/include/moveit/ompl_interface/detail/same_shared_ptr.hpp

I think I'm also leaning towards not adding these typedefs. They don't really hurt, but they can't be removed later without breaking backwards compatibility either.

@davetcoleman
Copy link
Copy Markdown
Member

davetcoleman commented Sep 12, 2016

Does #215 replace or require this PR? It seems to me we decided these extra macros in this PR are not needed?

@de-vri-es
Copy link
Copy Markdown
Contributor Author

de-vri-es commented Sep 12, 2016

I think so. I just don't think there is enough justification to add these typedefs. And it looks like nobody else thinks they're needed either. I think this can be closed.

@v4hn v4hn closed this Sep 13, 2016
@v4hn
Copy link
Copy Markdown
Contributor

v4hn commented Sep 13, 2016

If somebody is interested in this, feel free to comment here. Closed for now

@de-vri-es de-vri-es mentioned this pull request Sep 22, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@de-vri-es @davetcoleman @v4hn