Repository navigation
Conversation
Use the result register as the Inc/Dec source so postfix expressions return the converted old value while the local receives the new value. Preserve the existing two-instruction sequence and add regression coverage for coercion and abrupt completion.
Avoid clearing the source register with take_register before calling to_numeric() so discarded (n++/n--) and prefix (++n/--n) local updates preserve their binding if ToNumeric throws.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Local postfix updates can return the original value instead of its numeric conversion. For example, after
let n = '1'; const old = n++;,oldis incorrectly a string. The same problem affects decrement and function parameters. Additionally, becauseIncandDecpreviously took the source register withtake_registerbefore callingto_numeric(), any local update whose source operand was the local register (n++;,++n;,const next = ++n;) cleared the local binding toundefinedifToNumericthrew an exception.Related to #1913, which documents the required
ToNumeric(oldValue)result.It changes the following:
Move(result, local); Inc(local, result)(orDec) so the opcode converts the result register and writes the new value to the local. Previously it converted the local in place and left the result register holding the unconverted value.get_register(src.into()).clone()inInc::operationandDec::operation(matchingPos,Neg,BitNot, andToInt32) instead oftake_register(src.into())so that an abrupt completion into_numeric()does not overwrite a local register withundefined.const old = n++,n++) and prefix (const next = ++n,++n) updates.The tests were added before changing production code and failed on unmodified main
f0789a20:One failing JavaScript assertion was:
And for abrupt completion on a discarded or prefix local update:
Both return
truewith the fix. All six new tests (1144unit tests inboa_engine) now pass.Cost: the optimized path still emits two instructions and uses the existing result register. This adds no bytecode instruction or temporary register.
IncandDeccloneJsValuefrom the source register beforeto_numeric()instead of taking it out of the register slot. Runtime performance was not benchmarked.Validation on Rust 1.99.0:
cargo +1.99.0 test -p boa_engine --lib: 1,144 passed, 0 failed, 0 ignored.cargo +1.99.0 clippy -p boa_engine --lib --tests -- -D warnings: passed.cargo +1.99.0 fmt --check: passed.