-
Notifications
You must be signed in to change notification settings - Fork 434
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
Made options optional. Updated API documentation. #775
Conversation
Signed-off-by: Esteve Fernandez <esteve@apache.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The current change is not ABI compatible and can therefore not be backported to Dashing as-is. @ivanpauno was this selected for a specific use case to be backported? The simple workaround seems to be to just pass the default value explicitly. @esteve @ivanpauno If either of you still thinks this should be backported can you please create a custom PR targeting the |
@dirk-thomas How does this break ABI? It looks like the constructor has the same number of arguments with the same types as before. |
I think that usually defaults are implemented as temporary objects created at the point of call, and it calls the same symbol when using the default than when passing the parameter explicitly. In the first case this would be ABI compatible, in the second not. For me, it's okay not doing a backport. |
I couldn't come up with a reference. With
The second case would add a new (non-virtual) constructor with one argument less. That change should be ABI compatible too. I will take back my objection about the ABI compatibility then. The other question remains though: why should this be backported? |
There's not good reason, so I think not doing the backport is fine. |
I don't really see why not, it'd make the lifecycle API consistent with the node API, it's not breaking the ABI and also fixes the documentation. |
…I documentation. (#775) Signed-off-by: Esteve Fernandez <esteve@apache.org>
Since there is another backport for this repo I don't mind to also pick this one, see #801. |
Signed-off-by: Stephen Brawner <brawner@gmail.com>
* Add callbacks to the jump API Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Add unit tests for TimeControllerClock::jump callbacks Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Address some code style issues and renames Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Change Doxygen comments to `C` style. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Allow one of the callbacks to be nullptr. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
This PR updates the constructor to match that of
Node
(i.e.options
should be optional). I also removed part of the API documentation that was no longer relevant.