briansmith / ring

Safe, fast, small crypto using Rust
Other
3.76k stars 708 forks source link

Remove needless as_bytes() call #2147

Closed samueltardieu closed 1 month ago

samueltardieu commented 1 month ago

String::len() already returns the number of bytes in the string.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.03%. Comparing base (7c0024a) to head (bed207e). Report is 7 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2147 +/- ## ========================================== - Coverage 97.12% 97.03% -0.10% ========================================== Files 151 151 Lines 20101 19602 -499 Branches 447 447 ========================================== - Hits 19524 19020 -504 - Misses 482 487 +5 Partials 95 95 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

briansmith commented 1 month ago

Thanks for the PR!

String::len() already returns the number of bytes in the string.

This is true but I would argue that the way the code is currently written is clearer and perhaps {str/String}::len() shouldn't exist. Since the current code passes Clippy I am inclined to leave it as-is and I hope Clippy doesn't start warning about this pattern.

WDYT?

samueltardieu commented 1 month ago

I totally agree that {String, str}::len() should not exist in the first place. But they do, and I think using .as_bytes().len() adds to the confusion, not at the place where it is used, but everywhere else: it may let people think that this is necessary, and that len() alone would return the number of codepoints.

This is why I prefer to use the function directly and have a clippy lint (which can be disabled of course, at a local or project level) to warn people that this is unnecessary because precisely len() is weirdly defined on strings.

briansmith commented 1 month ago

I think this is just a matter of taste that's not worth working on so I'm going to close it. I appreciate the effort though.