Page MenuHomePhabricator

Expand IsOpcodeDisabled() function to prepare for implementation of re-enabled opcodes in Magnetic update.
AbandonedPublic

Authored by danconnolly on Jul 25 2018, 10:24.

Details

Reviewers
schancel
jasonbcox
Group Reviewers
Restricted Project
Summary

The purpose of this change is to prepare for the implementation of the re-enabled opcodes in the Magnetic update. The IsOpcodeDisabled() function is altered to add a check for the SCRIPT_ENABLE_MAGNETIC_OPCODES flag and the opcodes that will probably be re-enabled.

This check is structured in such a way that future changes can implement the re-enabled opcodes individually and independently.

The change does not in itself result in any behavioural change. We have verified that the tests in script_tests.json cover these changes and since the tests have not been altered, their continued success verifies that the behaviour of the function has not changed.

Test Plan

make check

Diff Detail

Branch
opcodes_2
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3038
Build 4168: arc lint + arc unit

Event Timeline

danconnolly created this revision.Jul 25 2018, 10:24
danconnolly retitled this revision from expand isdisabledopcode function to enable implementation of magnetic opcodes to Expand IsOpcodeDisabled() function to prepare for implementation of re-enabled opcodes in Magnetic update..Jul 28 2018, 10:10
danconnolly edited the summary of this revision. (Show Details)
danconnolly added a reviewer: Restricted Project.Jul 28 2018, 12:49
schancel requested changes to this revision.Aug 2 2018, 21:12
schancel added a subscriber: schancel.
schancel added inline comments.
src/script/interpreter.cpp
91 ↗(On Diff #4395)

These aren't currently disabled are they? They're invalid opcodes I thought?

This revision now requires changes to proceed.Aug 2 2018, 21:12
danconnolly added inline comments.Aug 2 2018, 22:13
src/script/interpreter.cpp
91 ↗(On Diff #4395)

Nope, disabled.

danconnolly requested review of this revision.Aug 3 2018, 21:39

The opcodes were disabled, not invalid. See lines 85-93 in interpreter.cpp

jasonbcox requested changes to this revision.Aug 6 2018, 15:52
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/script/interpreter.cpp
93 ↗(On Diff #4395)

We had discussed removing 2MUL and 2DIV. I don't see value in adding them if we're trying to keep a reduced instruction set.

This revision now requires changes to proceed.Aug 6 2018, 15:52
danconnolly marked an inline comment as done.Aug 8 2018, 19:05
danconnolly added inline comments.
src/script/interpreter.cpp
93 ↗(On Diff #4395)

Updated spec, will remove them.

danconnolly updated this revision to Diff 4529.Aug 8 2018, 19:08
danconnolly marked 4 inline comments as done.
  • specification has been updated to remove OP_2MUL & OP_2DIV
schancel added a comment.Aug 9 2018, 01:06

@danconnolly You're correct, my bad. This looks good. Let me take a look at the implementations before I sign off on landing this.

danconnolly abandoned this revision.Sep 4 2018, 09:29