Change all std::unique_ptr members of *Private classes to std::optional #472
Conversation
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]>
| @@ -248,97 +204,97 @@ void Geometry::SetType(const GeometryType _type) | |||
| ///////////////////////////////////////////////// | |||
| const Box *Geometry::BoxShape() const | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I would love to do that, but I think that would break existing code without a tick-tock.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, I think we'd have to do that. Not sure if I want to tackle that right now.
Signed-off-by: Addisu Z. Taddese <[email protected]>
|
@osrf-jenkins retest this please |
This avoids having to create copy constructors for those
*Privateclasses and makes way for usingignition::utils::ImplPtrToward #467