Closed Kalkwst closed 1 year ago
All modified lines are covered by tests :white_check_mark:
Comparison is base (
e5cd1f5
) 95.90% compared to head (9c6b527
) 95.92%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
While Codacy might flag the following operation as unnecessary,
var isZeroMask = ((currentByte ^ 0x00) - 1) >> 31;
it's actually a bit manipulation trick often used in low-level or performance-critical code. Here's what it does:
currentByte ^ 0x00
: This is a bitwise XOR operation with 0x00
. Any value XORed with 0x00
remains unchanged. This might seem redundant, but it's often used for clarity to indicate that a bitwise operation is taking place.(currentByte ^ 0x00) - 1
: Subtracting 1
results in all bits of the value being flipped if the original value was 0
. For any other value, not all bits will be flipped.((currentByte ^ 0x00) - 1) >> 31
: This is a right shift operation. If the original value was 0
, we now have a number with all bits set to 1
, and shifting 31
places to the right will result in a number with its only bit set to 1
(or -1
in signed integer representation). For any other original value, the result of the shift operation will be 0
.So, this operation essentially checks if currentByte
is zero. If it is, isZeroMask
will be -1
. Otherwise, it will be 0
.
While this might seem more complex than simply using an equality check like currentByte == 0
, these kinds of bit manipulation tricks can sometimes be faster than branching operations like if-else statements. They allow for more predictable execution paths and better CPU pipeline utilization.
Given that this algorithm demonstrates cryptographic padding, avoiding branching is crucial to prevent timing attacks in cryptographic contexts. Despite this implementation being less straightforward, it accurately reflects how we would implement this padding in a real-world cryptographic context. Therefore, we should disregard Codacy's issue in this instance.
I'm fine with the mask, but currentByte ^ 0x00
is redundant as you have mentioned, and it's not clear to me what indicate that a bitwise operation is taking place
means. Subtraction of a 1 is not a bitwise operation, and shift is clearly a bitwise operation by itself, there's no need to specifically mark it as such.
I'd say that removing xor simplifies the expression and makes it less confusing.
This pull request introduces a new class that implements the
ISO 7816-4
padding algorithm. The class provides three main methods:AddPadding
,RemovePadding
, andGetPaddingSize
.AddPadding
: This method adds padding to the end of a byte array according to theISO 7816-4
standard.RemovePadding
: This method removes theISO 7816-4
padding from the given input data.GetPaddingSize
: This method gets the number of padding bytes in the given input data according to theISO 7816-4
padding scheme.[x] I have performed a self-review of my code
[x] My code follows the style guidelines of this project
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[x] Comments in areas I changed are up to date
[x] I have added comments to hard-to-understand areas of my code
[x] I have made corresponding changes to the README.md