Removing magic numbers from drivers sample #41
Conversation
sanjm25
force-pushed
the
refactor/remove-magic-numbers
branch
from
ef1d3c7 to
8cef9a9
Compare
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.
| @@ -153,9 +158,9 @@ Exit the app anytime by pressing Ctrl-C | |||
|
|
|||
| 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, 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, |
| @@ -79,7 +85,7 @@ fn echo_interlocked_increment_floor(target: &AtomicI32, floor: i32) -> i32 { | |||
| // | |||
| 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
| @@ -727,3 +733,7 @@ unsafe extern "C" fn echo_evt_timer_func(timer: WDFTIMER) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // 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
| @@ -162,7 +166,7 @@ extern "C" fn echo_evt_device_self_managed_io_start(device: WDFDEVICE) -> NTSTAT | |||
| // into low power state. | |||
| 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()
| @@ -108,7 +112,7 @@ pub fn echo_device_create(mut device_init: &mut WDFDEVICE_INIT) -> NTSTATUS { | |||
| // it will return NULL and assert if run under framework verifier mode. | |||
| 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
| @@ -266,7 +272,7 @@ fn echo_decrement_request_cancel_ownership_count(request_context: *mut RequestCo | |||
| .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
Converted magic numbers to const.