Removing magic numbers from drivers sample #41
Conversation
sanjm25
force-pushed
the
refactor/remove-magic-numbers
branch
from
ef1d3c7 to
8cef9a9
Compare
leon-xd
left a comment
leon-xd
left a comment
There was a problem hiding this comment.
looking through there is actually a decent amount of magic numbers that were missed -- I started logging all of them in queue.rs but kept finding more. think it's worth to just call that out and for you to manually check by hand whether there are magic numbers that you missed.
| h.join().unwrap().unwrap(); | ||
| } else { | ||
| perform_write_read_test(h_device, 512)?; | ||
| perform_write_read_test(h_device, u32::try_from(TEST_SIZE_SMALL).unwrap())?; |
There was a problem hiding this comment.
why is this const not defined as the type it needs to be?
| perform_write_read_test(h_device, u32::try_from(TEST_SIZE_SMALL).unwrap())?; | ||
|
|
||
| perform_write_read_test(h_device, 30 * 1024)?; | ||
| perform_write_read_test(h_device, u32::try_from(TEST_SIZE_LARGE).unwrap())?; |
There was a problem hiding this comment.
same thing here switch to proper type
| GLOBAL_BUFFER = ExAllocatePool2(POOL_FLAG_NON_PAGED, LENGTH as SIZE_T, 's' as u32); | ||
| GLOBAL_BUFFER = ExAllocatePool2( | ||
| POOL_FLAG_NON_PAGED, | ||
| POOL_ALLOCATION_SIZE as SIZE_T, |
| match target.compare_exchange( | ||
| current_value, | ||
| current_value + 1, | ||
| current_value + ATOMIC_INCREMENT_VALUE, |
There was a problem hiding this comment.
hm does incrementing by 1 count as a magic number? suppose this is ok but given the lack of ++ operator i can imagine this could detract from readability rather than expand it
| } | ||
|
|
||
| // Define constants for magic numbers | ||
| const TOLERABLE_DELAY: u32 = 0; |
There was a problem hiding this comment.
need to put these with other constants should not be at bottom of file
| unsafe { call_unsafe_wdf_function_binding!(WdfIoQueueStart, queue) }; | ||
|
|
||
| let due_time: i64 = -(100) * (10000); | ||
| let due_time: i64 = TIMER_DUE_TIME_MS; |
There was a problem hiding this comment.
we should just use TIMER_DUE_TIME_MS instead of due_time in the call to timer.start()
| let device_context: *mut DeviceContext = | ||
| unsafe { wdf_object_get_device_context(device as WDFOBJECT) }; | ||
| unsafe { (*device_context).private_device_data = 0 }; | ||
| unsafe { (*device_context).private_device_data = DEFAULT_PRIVATE_DATA }; |
There was a problem hiding this comment.
what is the type of private_device_data? doesn't feel like this should be expressed as a number
| let result = unsafe { | ||
| (*request_context) | ||
| .cancel_completion_ownership_count | ||
| .fetch_sub(1, Ordering::SeqCst) |
There was a problem hiding this comment.
missed a magic number here
There was a problem hiding this comment.
also missed in echo_increment_request_cancel_ownership_count for some reason I can't write a comment there
There was a problem hiding this comment.
and also echo_set_current_request
There was a problem hiding this comment.
actually two of them in echo_set_current_request **
There was a problem hiding this comment.
there were too many to log in this comment chain so I submitted the review. Please check by hand for magic numbers
2 participants
Converted magic numbers to const.