refactor: remove magic numbers#64
Conversation
…s into function scope as appropriate,
| clippy::cast_possible_truncation, | ||
| reason = "100ms in 100-nanosecond units is known to fit in i64" | ||
| )] | ||
| const WDF_REL_TIMEOUT_100_MS: i64 = { |
There was a problem hiding this comment.
It's was initially unclear to me what this value is supposed to be for. It would be nice to have it so I didn't need to look at https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdftimer/nf-wdftimer-wdftimerstart docs to understand what value this is. Some things to consider are better name (WDF_TIMER_DUE_TIME_100_MS so its clear this is a value used for wdf timers), SYSTEM_TIME_UNITS instead of UNITS with a short comment about what system time units are (100-nanosecond intervals)
| const { | ||
| assert!(UNITS <= i64::MAX as u128, "1,000,000 should fit in i64"); | ||
| }; | ||
| -(UNITS as i64) |
There was a problem hiding this comment.
Your negative sign comment should be here, not in the docstring for the value
| const WDF_REL_TIMEOUT_100_MS: i64 = { | ||
| const UNITS: u128 = Duration::from_millis(100).as_nanos() / 100; | ||
| const { | ||
| assert!(UNITS <= i64::MAX as u128, "1,000,000 should fit in i64"); |
There was a problem hiding this comment.
nit:
| assert!(UNITS <= i64::MAX as u128, "1,000,000 should fit in i64"); | |
| assert!(UNITS <= i64::MAX as u128, "UNITS should fit in i64"); |
There was a problem hiding this comment.
Changed to naming convention mentioned above: SYSTEM_TIME_UNITS
| unsafe { | ||
| (*request_context).cancel_completion_ownership_count = AtomicI32::new(1); | ||
| (*request_context).cancel_completion_ownership_count = | ||
| AtomicI32::new(INITIAL_CANCEL_OWNERSHIP_COUNT); |
There was a problem hiding this comment.
this is fine to keep as literal
There was a problem hiding this comment.
Reverted change back to literal
| const TIMER_PERIOD_10_S: u32 = { | ||
| const MILLIS: u128 = Duration::from_secs(10).as_millis(); | ||
| const { | ||
| assert!(MILLIS <= u32::MAX as u128, "10,000 should fit in u32"); |
There was a problem hiding this comment.
Changed to a literal with a comment
| (*request_context) | ||
| .cancel_completion_ownership_count | ||
| .fetch_sub(2, Ordering::SeqCst); | ||
| .fetch_sub(TIMER_CLAIMED_OWNERSHIP_COUNT, Ordering::SeqCst); |
There was a problem hiding this comment.
The point of this is to make code more readable. I'm not sure this one does that. I think just cleaning up the comment to be clearer and a 2 should be fine for this one
- revert unnecessary const's to literals w/improved comments
| Size: WDF_TIMER_CONFIG_SIZE, | ||
| EvtTimerFunc: Some(echo_evt_timer_func), | ||
| Period: TIMER_PERIOD, | ||
| Period: 10_000, // 10 seconds, in milliseconds |
There was a problem hiding this comment.
It was requested to keep these as literals:
#64 (comment)
#64 (comment)
3 participants
Summary
Decompose magic numbers into
const's adding relevant documentation.