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

Refactor timer callback API to support context parameter #241

Merged
merged 7 commits into from Jan 15, 2022

Conversation

meeq
Copy link
Contributor

@meeq meeq commented Jan 11, 2022

This approach uses an anonymous union of function pointers and a flag to select between callback signatures. Thanks for the advice @rasky

This approach attempts to maintain compatibility with existing usage by
introducing new functions with an updated signature, and replacing the
existing functions with an inline shim to use the new functions.

The old callback signature is made forward-compatible with the new
callback signature by casting the function pointer, which introduces
no overhead compared to a trampoline. This casting trick only works
because the new signature is the same as the old signature with an
extra argument at the end.
@meeq
Copy link
Contributor Author

meeq commented Jan 13, 2022

@anacierdem @rasky This PR has incorporated all of the feedback from Discord. Please let me know if you have any additional suggestions. Thanks!

@anacierdem
Copy link
Collaborator

LGTM. Adding a simple test for context version would be awesome though 😊

@meeq
Copy link
Contributor Author

meeq commented Jan 13, 2022

@anacierdem Adding a simple test for context version would be awesome though 😊

Done in 24e7c02

@@ -189,22 +189,23 @@ static const struct Testsuite
uint32_t duration;
uint32_t flags;
} tests[] = {
TEST_FUNC(test_exception, 5, TEST_FLAGS_NO_BENCHMARK),
Copy link
Contributor Author

@meeq meeq Jan 13, 2022

Choose a reason for hiding this comment

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

This block was a mix of tabs and spaces, which was causing visual alignment issues. I have made it consistent regardless of tab size by using a leading tab (no change), and spaces for field alignment padding.

Copy link
Collaborator

@rasky rasky left a comment

Choose a reason for hiding this comment

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

Thanks! PR looks OK. I left a small comment about docs, and then as @anacierdem said we would need at least some basic coverage test. You don't need to test in details all functionalities, that is already done with the previous callback. Just write a test that calls each function once and makes sure it doesn't crash.

* @param[in] ctx
* User data to pass as an argument to callback
*
* @return A pointer to the timer structure created
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would modify the existing documentation to mention that using the context version is possibly preferred. Also please add @see tag to reference related functions (context vs non-context).

I wouldn't mark the non-context version as deprecated or using a too strong wording. As long as we point new users to the new versions, I think the older versions are fine to stay without scaring too much.

@rasky rasky merged commit 38edd09 into DragonMinded:trunk Jan 15, 2022
@meeq meeq deleted the timer_ctx branch January 15, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants