Add DART_COMMON_DECLARE_SMART_POINTERS macro

[jslee02]

Summary

• Add missing std::unique_ptr declaration to the pointer declaration macro
• Use using instead of `typedef
• Rename DART_COMMON_MAKE_SHARED_WEAK to DART_COMMON_MAKE_POINTERS

If this change makes sense, I will replace the use of DART_COMMON_MAKE_SHARED_WEAK to the new macro.

---

Before creating a pull request

• Document new methods and classes
• Format new code files using clang-format

Before merging a pull request

• Set version target by selecting a milestone on the right side
• Summarize this change in CHANGELOG.md

[mxgrey]

I don't think we should ever completely remove DART_COMMON_MAKE_SHARED_WEAK, because some classes (at a minimum, the Skeleton class) must be instantiated into a std::shared_ptr in order to work correctly within DART's overall design. A std::unique_ptr<Skeleton> instance can never be safely supported. There are too many faculties of BodyNode and Node that require their Skeleton to be a shared resource.

Or if we do remove DART_COMMON_MAKE_SHARED_WEAK, we should not use DART_COMMON_MAKE_POINTERS on Skeleton, and instead just manually declare those pointers.

[jslee02]

@mxgrey That makes sense, I missed the fact that we have some classes shouldn't be used with std::unique_ptr. Let's not to remove the original macro.

Instead, how about following changes:

• Add DART_COMMON_MAKE_SHARED_WEAK_UNIQUE or DART_COMMON_MAKE_POINTERS that defines all the pointer types
• Use using instead of typedef in DART_COMMON_MAKE_SHARED_WEAK for consistency?

[mxgrey]

The more I think about it, the less convinced I am of this direction. There are also classes like Shape and SimpleFrame which don't technically need to be managed by a std::shared_ptr, but they are almost exclusively managed by std::shared_ptr within DART, because these are resources which often need to be shared. It wouldn't be strictly incorrect to manage them with std::unique_ptr, but users would frequently experience friction with DART's design if they tried to do so.

Consequently, I'm worried that natively providing something like a UniqueShapePtr or UniqueSimpleFramePtr would mislead users into thinking that these std::unique_ptr types play a role somewhere within DART, even though they really don't.

If a user has a specific purpose in mind for which they want to manage a resource with a std::unique_ptr, there is nothing to stop them from doing so (with the exception of classes like Skeleton, whose factory function only returns std::shared_ptr types).

Is there a specific motivation that I'm overlooking for trying to declare std::unique_ptr types for as many classes as possible? Of course I have nothing inherently against std::unique_ptr, and I recognize that it should generally be preferred as a default resource manager over std::shared_ptr. But in the specific context of dynamics and simulation, it's often more important that resources be shareable instead of uniquely owned.

[jslee02]

They are all valid points. I intend to use the new macro only to where it makes sense and won't replace DART_COMMON_MAKE_SHARED_WEAK with the new one unless there is a compelling reason.

The actual motivations of this change are (1) to use using instead of typedef consistently for type aliasing, (2) to avoid inconsistent naming of the pointer types (e.g., ConstUniqueXPtr (const and unique are swapped)), and (3) for less typing. 😄

[codecov]

Codecov Report

Merging #1022 into release-6.4 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release-6.4    #1022   +/-   ##
============================================
  Coverage        56.58%   56.58%           
============================================
  Files              314      314           
  Lines            24312    24312           
============================================
  Hits             13758    13758           
  Misses           10554    10554

[jslee02]

@mxgrey By the last commits, this PR only (1) adds a new macro DART_COMMON_MAKE_POINTERS that declares all the three types of STL pointer types and (2) replaces typedef with using in DART_COMMON_MAKE_SHARED_WEAK.

DART_COMMON_MAKE_POINTERS is not to replace DART_COMMON_MAKE_SHARED_WEAK but will be used in case we want to declare all the smart pointer types with the same naming fashion with DART_COMMON_MAKE_SHARED_WEAK.

Let me know what you think.

[mxgrey]

We could consider naming the macro DART_COMMON_MAKE_SMART_POINTERS, but I approve either way.

I sort of regret that I didn't initially name the macro DART_COMMON_DECLARE_SHARED_WEAK instead of saying ..._MAKE_.... I think we should consider changing it to say _DECLARE_ instead, but it's very much not important.

[jslee02]

I also slightly prefer using _DECLARE_ rather than _MAKE_. How about we use _DECLARE_ and deprecate (it would be just a comment because there is no way to deprecate a macro) _MAKE_?

[mxgrey]

Changing the macro name and deprecating the old one sounds good to me!

Making the "deprecation" warning cross-platform (i.e. work on Windows) is a little tricky, though.