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

set_alarm: Misc fixes and improvements #77

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

N-R-K
Copy link
Contributor

@N-R-K N-R-K commented Oct 25, 2022

I noticed a couple things going on in set_alarm which seemed dubious. Here's a summary of the changes (additional context provided in commit message). Feel free to cherry-pick in case you don't agree with a certain commit.

  • value was unused, remove it.
  • value_type was set in attr but the necessary value_mask wasn't set on flags. So add XSyncCAValueType to flags.
  • Instead of destroying and creating alarm, just use XSyncChangeAlarm to change the existing alarm.
  • Ensure multiplication takes place in unsigned long instead of casting the result (otherwise that expression would always result in 0).

value_type was being set in `attr` but it wasn't set in `flags`.
the cast here seems dobious. the multiplication will take place in
`unsigned int` and the cast only affects the _result_ and not the
multiplication itself. which means that the high 32bits will remain zero
and the shift will thus always produce zero as well.

use UL suffix on the integer constant to ensure that the multiplication
itself happens at `unsigned long`.
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.

1 participant