Conversation
88b914f to
9b10d9b
Compare
cfsmp3
left a comment
There was a problem hiding this comment.
Thanks for working on cleaning up clippy pedantic warnings!
However, I found a few issues with this PR:
1. Incorrect claim about warnings
The PR states:
cargo clippy -- -W clippy::pedanticno longer shows warnings for this file
This is not accurate. Running clippy with pedantic still shows 40+ warnings for track_lister.rs, including warnings for the exact same as u16 cast pattern that wasn't changed:
--> src/track_lister.rs:699:20
let pid = (((packet[offset + 1] & 0x1F) as u16) << 8) | packet[offset + 2] as u16;
--> src/track_lister.rs:725:44
let program_num = ((packet[payload_offset] as u16) << 8)
2. Incomplete fix
The PR only changes 3 locations but leaves identical patterns unchanged:
- Line 699: Same
as u16pattern - not changed - Lines 725-726: Same
as u16pattern - not changed
If we're fixing these casts, we should fix all of them for consistency.
3. Inconsistent mask ordering
At line 757, the mask is applied after conversion:
u16::from(packet[payload_offset + 8]) & 0x1F // convert first, then maskBut at lines 727 and 772, the mask is applied before:
u16::from(packet[payload_offset + 2] & 0x1F) // mask first, then convertWhile numerically equivalent, this inconsistency is confusing. Line 757 should be:
u16::from(packet[payload_offset + 8] & 0x1F) // consistent with othersSuggested fix
Please either:
- Fix all similar
as u16cast warnings in the file for consistency, or - Update the PR description to clarify this only addresses a subset of warnings
Also fix the mask ordering inconsistency at line 757.
6efac5d to
5988ad3
Compare
5988ad3 to
cad4d3d
Compare
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 2582f62...:
NOTE: The following tests have been failing on the master branch as well as the PR:
This PR does not introduce any new test failures. However, some tests are failing on both master and this PR (see above). Check the result page for more info. |
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit a30b8d7...:
NOTE: The following tests have been failing on the master branch as well as the PR:
This PR does not introduce any new test failures. However, some tests are failing on both master and this PR (see above). Check the result page for more info. |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Resolves several
clippy::pedanticwarnings insrc/track_lister.rsrelated to unsafe casting.Changes
as u16casts withu16::from()in 5 instances withintrack_lister.rs.Technical Rationale
While
u8tou16is a widening conversion, usingFromis preferred overasbecause:u32),Fromwill trigger a compile-time error rather than silently truncating bits.Verification
cargo checkpasses.cargo clippy -- -W clippy::pedanticno longer shows warnings for this file.cargo testpasses (395 tests passed).