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.

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

Conversation

azeey
Copy link
Collaborator

@azeey azeey commented Jan 20, 2021

This avoids having to create copy constructors for those *Private classes and makes way for using ignition::utils::ImplPtr

Toward #467

This avoids having to create copy constructors for those *Private
classes and makes way for using ignition::utils::ImplPtr

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey requested review from brawner and chapulina January 20, 2021 17:28
@azeey azeey self-assigned this Jan 20, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Jan 20, 2021
@azeey azeey added this to the SDFormat 1.8 / libsdformat11 milestone Jan 20, 2021
src/Geometry.cc Show resolved Hide resolved
@@ -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.

src/Utils.hh Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor

@osrf-jenkins retest this please

@azeey azeey merged commit a721bba into gazebosim:master Jan 21, 2021
@azeey azeey deleted the use_optional branch January 21, 2021 21:47
@azeey azeey mentioned this pull request Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants