Remove an impossible check in Reader.prototype.readLength#55
Remove an impossible check in Reader.prototype.readLength#55A-D-E-A wants to merge 1 commit intoTritonDataCenter:masterfrom
Reader.prototype.readLength#55Conversation
removes the impossible "if (lenB === null)" check, since it is impossible for it to be null because of the "& 0xff" on the previous line.
|
1.) This reads like an AI hallucination; it might even be one. 2.) Even if 1 is an overblown concern, I see no testing notes here. 3.) Speaking of testing, how did you encounter this problem in the first place? |
|
This is not an AI hallunication, sorry, I'm just not that good at writing. I'm not sure what testing notes you'd like. Basically, my tests consisted of the following: var toTest = [
[],
{},
Buffer.alloc(0),
0,
-0,
NaN,
null,
undefined,
[,][0],
/_/,
true,
false,
"str",
... // Many other things except Symbol and BigInt which crash
];
for (var v of toTest) {
if (v & 0xff === null)
console.log("Found a specific case in which the '=== null' is important");
}I tested that for many versions of node (I don't remember exactly which as I did it manually, but at least each major since 10 worked on my computer); none of which logged. I could make a setup to test each node version since 1 to be absolutely sure. I encountered the "problem" (not really a problem, just a weird and seemingly useless line) by reading the code. I don't know a lot about ASN.1 and BER, and I have to use it for an internal project. I often try to rewrite a library (my code and the official library side-by-side) in my codestyle to better understand the internals. I don't think a new test case/file is needed, because it would be more of a language implementation issue than a library one. But I might be wrong. |
|
We use this on versions of node well before 10 internally, so 10 or later is not sufficient. |
Removes the impossible "if (lenB === null)" check, since it is impossible for it to be null because of the "& 0xff" on the previous line.
According the the spec (https://262.ecma-international.org/#sec-numberbitwiseop), the result of
this._buf[offset++] & 0xff(call to the internalNumberBitwiseOp(&, this._buf[offset++], 0xff)) must be an integer unless it throws. It may throw becauseToInt32(https://262.ecma-international.org/#sec-toint32) callsToNumber(https://262.ecma-international.org/#sec-tonumber) which throws if the argument is a Symbol or BigInt. I tested many combinations ofthing & 0xffin multiple versions of Node; and have not yet found a case where this part of the spec is false.Since it may never be null, the lines 93 and 94 that say
if (lenB === null) return nullare basically a no-op.Removing it should not be a breaking change for anyone.
It's not much, but it may shave off a few bytes and CPU cycles.