Page MenuHomePhabricator

Adds OP_DIV functionality into interpreter.cpp
AbandonedPublic

Authored by schancel on Feb 15 2018, 00:52.

Details

Reviewers
jasonbcox
movrcx
Group Reviewers
Restricted Project
Restricted Owners Package(Owns No Changed Paths)
Summary

Re-implements OP_DIV (division by zero is not authorized)

Co-authored-by: Joshua Yabut <yabut.joshua@gmail.com>
Co-authored-by: Marcos Mayorga <mm@mm-studios.com>

Test Plan

Unit tests in test/op_code.cpp

make check

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1845
Build 1878: arc lint + arc unit

Event Timeline

schancel added inline comments.
src/script/script.h
277 ↗(On Diff #2897)

It would be nice if division by zero was in here, but I think throwing an exception would be bad.

src/script/script.h
277 ↗(On Diff #2897)

The div-by-zero check is in intepreter.cpp so in theory a zero should never be passed to this function. But that doesn't preclude the possibility of a future op code calling the div operator without a zero check. Given that in theory the exception would never be triggered unless someone makes a mistake in the future would an exception here to act as a guard be a problem? I don't know if C++ has the concept of checked and unchecked exceptions but if so I would suggest an unchecked exception.

Whatever happens here should happen to the mod operator as well.

jasonbcox requested changes to this revision.Feb 23 2018, 18:30
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/script/script.h
277 ↗(On Diff #2897)

Changing the implementation to:

return m_value / CScriptNum(rhs);

would call what I'd consider to be the "base" operator that's implemented below. Funneling calls into the same codepaths is generally good design as it discourages re-implementation errors. (for example, if you add a divide-by-zero check, you only need to add it to the division operator below).

This revision now requires changes to proceed.Feb 23 2018, 18:30
src/script/script.h
277 ↗(On Diff #2897)

Oops I had it backwards, the below operator actually calls this one. Nevermind!

schancel added inline comments.
src/script/script.h
277 ↗(On Diff #2897)

An exception could be equally as bad. Is an std::optional or std::variant a reasonable possibility?

Paging @deadalnix for an opinion.

src/script/script.h
277 ↗(On Diff #2897)

Gentle repaging @deadalnix for an opinion on whether and how to add another level of checking for div by zero here, in case the operator is used by some other code in the future.

src/script/script.h
281 ↗(On Diff #3145)

There was an open question here about whether to add an additional check for division by zero. In the current implementation, this is checked before this operator is used, however this cannot be guaranteed for future uses of this operator. For previous inline comments, see https://reviews.bitcoinabc.org/D1093?id=2897

src/script/script.h
281 ↗(On Diff #3145)

I spoke with deadalnix on this. It should not be necessary. Any exception would be inappropriate anyways. I think just having a test for this case is sufficient.

danconnolly edited the test plan for this revision. (Show Details)

resubmit OP_DIV opcode

Owners added a reviewer: Restricted Owners Package.Mar 12 2018, 16:59
schancel added a reviewer: movrcx.