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.

Sign up for GitHub

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

Change all std::unique_ptr members of *Private classes to std::optional #472

Merged
merged 3 commits into from
Jan 21, 2021
Merged

Change all std::unique_ptr members of *Private classes to std::optional #472

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ec5f5a
Change all std::unique_ptr members of *Private classes to std::optional
azeey Jan 14, 2021
2372125
Add documentation and const overload to sdf::optionalToPointer
azeey Jan 20, 2021
1f7d505
Merge remote-tracking branch 'upstream/master' into use_optional
azeey Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 46 additions & 90 deletions src/Geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
* limitations under the License.
*
*/

#include <optional>

#include "sdf/Geometry.hh"
#include "sdf/Box.hh"
#include "sdf/Capsule.hh"
Expand All @@ -24,6 +27,8 @@
#include "sdf/Plane.hh"
#include "sdf/Sphere.hh"

#include "Utils.hh"

using namespace sdf;

// Private data class
Expand All @@ -32,29 +37,29 @@ class sdf::GeometryPrivate
// \brief The geometry type.
public: GeometryType type = GeometryType::EMPTY;

/// \brief Pointer to a box.
public: std::unique_ptr<Box> box;
/// \brief Optional box.
public: std::optional<Box> box;

/// \brief Pointer to a capsule.
public: std::unique_ptr<Capsule> capsule;
/// \brief Optional capsule.
public: std::optional<Capsule> capsule;

/// \brief Pointer to a cylinder.
public: std::unique_ptr<Cylinder> cylinder;
/// \brief Optional cylinder.
public: std::optional<Cylinder> cylinder;

/// \brief Pointer to an ellipsoid
public: std::unique_ptr<Ellipsoid> ellipsoid;
/// \brief Optional ellipsoid
public: std::optional<Ellipsoid> ellipsoid;

/// \brief Pointer to a plane.
public: std::unique_ptr<Plane> plane;
/// \brief Optional plane.
public: std::optional<Plane> plane;

/// \brief Pointer to a sphere.
public: std::unique_ptr<Sphere> sphere;
/// \brief Optional sphere.
public: std::optional<Sphere> sphere;

/// \brief Pointer to a mesh.
public: std::unique_ptr<Mesh> mesh;
/// \brief Optional mesh.
public: std::optional<Mesh> mesh;

/// \brief Pointer to a heightmap.
public: std::unique_ptr<Heightmap> heightmap;
/// \brief Optional heightmap.
public: std::optional<Heightmap> heightmap;

/// \brief The SDF element pointer used during load.
public: sdf::ElementPtr sdf;
Expand All @@ -75,57 +80,8 @@ Geometry::~Geometry()

//////////////////////////////////////////////////
Geometry::Geometry(const Geometry &_geometry)
: dataPtr(new GeometryPrivate)
: dataPtr(new GeometryPrivate(*_geometry.dataPtr))
azeey marked this conversation as resolved.
Show resolved Hide resolved
{
this->dataPtr->type = _geometry.dataPtr->type;

if (_geometry.dataPtr->box)
{
this->dataPtr->box = std::make_unique<sdf::Box>(*_geometry.dataPtr->box);
}

if (_geometry.dataPtr->capsule)
{
this->dataPtr->capsule = std::make_unique<sdf::Capsule>(
*_geometry.dataPtr->capsule);
}

if (_geometry.dataPtr->cylinder)
{
this->dataPtr->cylinder = std::make_unique<sdf::Cylinder>(
*_geometry.dataPtr->cylinder);
}

if (_geometry.dataPtr->ellipsoid)
{
this->dataPtr->ellipsoid = std::make_unique<sdf::Ellipsoid>(
*_geometry.dataPtr->ellipsoid);
}

if (_geometry.dataPtr->plane)
{
this->dataPtr->plane = std::make_unique<sdf::Plane>(
*_geometry.dataPtr->plane);
}

if (_geometry.dataPtr->sphere)
{
this->dataPtr->sphere = std::make_unique<sdf::Sphere>(
*_geometry.dataPtr->sphere);
}

if (_geometry.dataPtr->mesh)
{
this->dataPtr->mesh = std::make_unique<sdf::Mesh>(*_geometry.dataPtr->mesh);
}

if (_geometry.dataPtr->heightmap)
{
this->dataPtr->heightmap =
std::make_unique<sdf::Heightmap>(*_geometry.dataPtr->heightmap);
}

this->dataPtr->sdf = _geometry.dataPtr->sdf;
}

//////////////////////////////////////////////////
Expand Down Expand Up @@ -176,56 +132,56 @@ Errors Geometry::Load(ElementPtr _sdf)
if (_sdf->HasElement("box"))
{
this->dataPtr->type = GeometryType::BOX;
this->dataPtr->box.reset(new Box());
this->dataPtr->box.emplace();
Errors err = this->dataPtr->box->Load(_sdf->GetElement("box"));
errors.insert(errors.end(), err.begin(), err.end());
}
else if (_sdf->HasElement("capsule"))
{
this->dataPtr->type = GeometryType::CAPSULE;
this->dataPtr->capsule.reset(new Capsule());
this->dataPtr->capsule.emplace();
Errors err = this->dataPtr->capsule->Load(_sdf->GetElement("capsule"));
errors.insert(errors.end(), err.begin(), err.end());
}
else if (_sdf->HasElement("cylinder"))
{
this->dataPtr->type = GeometryType::CYLINDER;
this->dataPtr->cylinder.reset(new Cylinder());
this->dataPtr->cylinder.emplace();
Errors err = this->dataPtr->cylinder->Load(_sdf->GetElement("cylinder"));
errors.insert(errors.end(), err.begin(), err.end());
}
else if (_sdf->HasElement("ellipsoid"))
{
this->dataPtr->type = GeometryType::ELLIPSOID;
this->dataPtr->ellipsoid.reset(new Ellipsoid());
this->dataPtr->ellipsoid.emplace();
Errors err = this->dataPtr->ellipsoid->Load(_sdf->GetElement("ellipsoid"));
errors.insert(errors.end(), err.begin(), err.end());
}
else if (_sdf->HasElement("plane"))
{
this->dataPtr->type = GeometryType::PLANE;
this->dataPtr->plane.reset(new Plane());
this->dataPtr->plane.emplace();
Errors err = this->dataPtr->plane->Load(_sdf->GetElement("plane"));
errors.insert(errors.end(), err.begin(), err.end());
}
else if (_sdf->HasElement("sphere"))
{
this->dataPtr->type = GeometryType::SPHERE;
this->dataPtr->sphere.reset(new Sphere());
this->dataPtr->sphere.emplace();
Errors err = this->dataPtr->sphere->Load(_sdf->GetElement("sphere"));
errors.insert(errors.end(), err.begin(), err.end());
}
else if (_sdf->HasElement("mesh"))
{
this->dataPtr->type = GeometryType::MESH;
this->dataPtr->mesh.reset(new Mesh());
this->dataPtr->mesh.emplace();
Errors err = this->dataPtr->mesh->Load(_sdf->GetElement("mesh"));
errors.insert(errors.end(), err.begin(), err.end());
}
else if (_sdf->HasElement("heightmap"))
{
this->dataPtr->type = GeometryType::HEIGHTMAP;
this->dataPtr->heightmap.reset(new Heightmap());
this->dataPtr->heightmap.emplace();
Errors err = this->dataPtr->heightmap->Load(_sdf->GetElement("heightmap"));
errors.insert(errors.end(), err.begin(), err.end());
}
Expand All @@ -248,97 +204,97 @@ void Geometry::SetType(const GeometryType _type)
/////////////////////////////////////////////////
const Box *Geometry::BoxShape() const
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could these getters now return a const std::optional<T> & or would that be too breaking of a change? It would probably save double checking if the optional type is valid. (once here in optionalToPointer and again at the calling site). It would just be pushed to the caller to check that the optional is valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even in this case, the caller has to check that the returned pointer is valid, so I don't think it's too much of a stretch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would love to do that, but I think that would break existing code without a tick-tock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that's a desirable change, then it seems like it makes sense to start the tick-tock cycle. But how would that look if you're just changing the return type? Would you need to create new method name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think we'd have to do that. Not sure if I want to tackle that right now.

{
return this->dataPtr->box.get();
return optionalToPointer(this->dataPtr->box);
}

/////////////////////////////////////////////////
void Geometry::SetBoxShape(const Box &_box)
{
this->dataPtr->box = std::make_unique<Box>(_box);
this->dataPtr->box = _box;
}

/////////////////////////////////////////////////
const Sphere *Geometry::SphereShape() const
{
return this->dataPtr->sphere.get();
return optionalToPointer(this->dataPtr->sphere);
}

/////////////////////////////////////////////////
void Geometry::SetSphereShape(const Sphere &_sphere)
{
this->dataPtr->sphere = std::make_unique<Sphere>(_sphere);
this->dataPtr->sphere = _sphere;
}

/////////////////////////////////////////////////
const Capsule *Geometry::CapsuleShape() const
{
return this->dataPtr->capsule.get();
return optionalToPointer(this->dataPtr->capsule);
}

/////////////////////////////////////////////////
void Geometry::SetCapsuleShape(const Capsule &_capsule)
{
this->dataPtr->capsule = std::make_unique<Capsule>(_capsule);
this->dataPtr->capsule = _capsule;
}

/////////////////////////////////////////////////
const Cylinder *Geometry::CylinderShape() const
{
return this->dataPtr->cylinder.get();
return optionalToPointer(this->dataPtr->cylinder);
}

/////////////////////////////////////////////////
void Geometry::SetCylinderShape(const Cylinder &_cylinder)
{
this->dataPtr->cylinder = std::make_unique<Cylinder>(_cylinder);
this->dataPtr->cylinder = _cylinder;
}

/////////////////////////////////////////////////
const Ellipsoid *Geometry::EllipsoidShape() const
{
return this->dataPtr->ellipsoid.get();
return optionalToPointer(this->dataPtr->ellipsoid);
}

/////////////////////////////////////////////////
void Geometry::SetEllipsoidShape(const Ellipsoid &_ellipsoid)
{
this->dataPtr->ellipsoid = std::make_unique<Ellipsoid>(_ellipsoid);
this->dataPtr->ellipsoid = _ellipsoid;
}

/////////////////////////////////////////////////
const Plane *Geometry::PlaneShape() const
{
return this->dataPtr->plane.get();
return optionalToPointer(this->dataPtr->plane);
}

/////////////////////////////////////////////////
void Geometry::SetPlaneShape(const Plane &_plane)
{
this->dataPtr->plane = std::make_unique<Plane>(_plane);
this->dataPtr->plane = _plane;
}

/////////////////////////////////////////////////
const Mesh *Geometry::MeshShape() const
{
return this->dataPtr->mesh.get();
return optionalToPointer(this->dataPtr->mesh);
}

/////////////////////////////////////////////////
void Geometry::SetMeshShape(const Mesh &_mesh)
{
this->dataPtr->mesh = std::make_unique<Mesh>(_mesh);
this->dataPtr->mesh = _mesh;
}

/////////////////////////////////////////////////
const Heightmap *Geometry::HeightmapShape() const
{
return this->dataPtr->heightmap.get();
return optionalToPointer(this->dataPtr->heightmap);
}

/////////////////////////////////////////////////
void Geometry::SetHeightmapShape(const Heightmap &_heightmap)
{
this->dataPtr->heightmap = std::make_unique<Heightmap>(_heightmap);
this->dataPtr->heightmap = _heightmap;
}

/////////////////////////////////////////////////
Expand Down
48 changes: 5 additions & 43 deletions src/Joint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,6 @@ using namespace sdf;

class sdf::JointPrivate
{
public: JointPrivate()
{
// Initialize here because windows does not support list initialization
// at member initialization (ie ... axis = {{nullptr, nullpter}};).
this->axis[0] = nullptr;
this->axis[1] = nullptr;
}

/// \brief Copy constructor
/// \param[in] _jointPrivate JointPrivate to copy.
public: explicit JointPrivate(const JointPrivate &_jointPrivate);

// Delete copy assignment so it is not accidentally used
public: JointPrivate &operator=(const JointPrivate &_) = delete;

/// \brief Name of the joint.
public: std::string name = "";

Expand All @@ -70,7 +55,7 @@ class sdf::JointPrivate

/// \brief Joint axis
// cppcheck-suppress
public: std::array<std::unique_ptr<JointAxis>, 2> axis;
public: std::array<std::optional<JointAxis>, 2> axis;

/// \brief The SDF element pointer used during load.
public: sdf::ElementPtr sdf;
Expand All @@ -82,28 +67,6 @@ class sdf::JointPrivate
public: sdf::ScopedGraph<sdf::PoseRelativeToGraph> poseRelativeToGraph;
};

/////////////////////////////////////////////////
JointPrivate::JointPrivate(const JointPrivate &_jointPrivate)
: name(_jointPrivate.name),
parentLinkName(_jointPrivate.parentLinkName),
childLinkName(_jointPrivate.childLinkName),
type(_jointPrivate.type),
pose(_jointPrivate.pose),
poseRelativeTo(_jointPrivate.poseRelativeTo),
threadPitch(_jointPrivate.threadPitch),
sdf(_jointPrivate.sdf),
frameAttachedToGraph(_jointPrivate.frameAttachedToGraph),
poseRelativeToGraph(_jointPrivate.poseRelativeToGraph)
{
for (std::size_t i = 0; i < _jointPrivate.axis.size(); ++i)
{
if (_jointPrivate.axis[i])
{
this->axis[i] = std::make_unique<JointAxis>(*_jointPrivate.axis[i]);
}
}
}

/////////////////////////////////////////////////
Joint::Joint()
: dataPtr(new JointPrivate)
Expand Down Expand Up @@ -232,14 +195,14 @@ Errors Joint::Load(ElementPtr _sdf)

if (_sdf->HasElement("axis"))
{
this->dataPtr->axis[0].reset(new JointAxis());
this->dataPtr->axis[0].emplace();
Errors axisErrors = this->dataPtr->axis[0]->Load(_sdf->GetElement("axis"));
errors.insert(errors.end(), axisErrors.begin(), axisErrors.end());
}

if (_sdf->HasElement("axis2"))
{
this->dataPtr->axis[1].reset(new JointAxis());
this->dataPtr->axis[1].emplace();
Errors axisErrors = this->dataPtr->axis[1]->Load(_sdf->GetElement("axis2"));
errors.insert(errors.end(), axisErrors.begin(), axisErrors.end());
}
Expand Down Expand Up @@ -338,14 +301,13 @@ void Joint::SetChildLinkName(const std::string &_name)
/////////////////////////////////////////////////
const JointAxis *Joint::Axis(const unsigned int _index) const
{
return this->dataPtr->axis[std::min(_index, 1u)].get();
return optionalToPointer(this->dataPtr->axis[std::min(_index, 1u)]);
}

/////////////////////////////////////////////////
void Joint::SetAxis(const unsigned int _index, const JointAxis &_axis)
{
this->dataPtr->axis[std::min(_index, 1u)] =
std::make_unique<JointAxis>(_axis);
this->dataPtr->axis[std::min(_index, 1u)] = _axis;
}

/////////////////////////////////////////////////
Expand Down
Loading