Skip to content

Comments

refactor: remove redundant "& 0xFF" masks from ord() and BitArray#203

Merged
DASPRiD merged 2 commits intoBacon:mainfrom
vlakoff:cleanup-0xff-mask
Nov 16, 2025
Merged

refactor: remove redundant "& 0xFF" masks from ord() and BitArray#203
DASPRiD merged 2 commits intoBacon:mainfrom
vlakoff:cleanup-0xff-mask

Conversation

@vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Nov 14, 2025

This change removes unnecessary bitmask operations that were carried over from C-style code:

  • In C, & 0xFF is used to prevent sign-extension when promoting signed chars (-128..127) to ints, ensuring values are treated as unsigned bytes (0..255).
  • In PHP, ord() already returns integers in the range 0–255, so applying & 0xFF is redundant.
  • Likewise, BitArray::toBytes() produces values in the correct range, making the extra mask in BitArray::generateEcBytes() unnecessary.

This simplifies the codebase and makes the implementation more idiomatic to PHP.

@DASPRiD
Copy link
Member

DASPRiD commented Nov 15, 2025

Please rebase against main to get the tests working.

These masks were carried over from C-style code, where "& 0xFF" is used to
prevent sign-extension when promoting signed chars (-128..127) to ints. In that
context, masking ensures the value is treated as an unsigned byte (0..255).

In PHP, however, ord() already returns an integer in the range 0–255, making
the extra "& 0xFF" operation unnecessary.
The mask was carried over from C-style code, where "& 0xFF" is used to prevent
sign-extension when promoting signed chars (-128..127) to ints.

In PHP, however, BitArray::toBytes() already produces values in the range 0–255,
making the extra mask operation unnecessary.
@vlakoff
Copy link
Contributor Author

vlakoff commented Nov 16, 2025

Rebased. Please let me know what you would like to see improved (including commit messages) before considering this for merging.

@codecov
Copy link

codecov bot commented Nov 16, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.58%. Comparing base (bd2370d) to head (7ae7f05).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Encoder/Encoder.php 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main     #203   +/-   ##
=========================================
  Coverage     70.58%   70.58%           
  Complexity      995      995           
=========================================
  Files            49       49           
  Lines          3182     3182           
=========================================
  Hits           2246     2246           
  Misses          936      936           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DASPRiD
Copy link
Member

DASPRiD commented Nov 16, 2025

LGTM, thanks!

@DASPRiD DASPRiD merged commit 8fa86d0 into Bacon:main Nov 16, 2025
9 checks passed
@vlakoff vlakoff deleted the cleanup-0xff-mask branch November 16, 2025 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants