feat: initializing conditional stack opcodes #32
Conversation
| if (self.stack.items.len == 0) { | ||
| return ConditionalStackError.EmptyConditionalStack; | ||
| } | ||
| _ = self.stack.pop(); |
There was a problem hiding this comment.
Why don't you return the value?
Should it be named delete then? Not pop
|
@okhaimie-dev don't forget to ask for a new review when you are finished pushing your changes |
src/script/cond_stack.zig
Outdated
| /// | ||
| /// Returns: | ||
| /// The popped value (u8) if the stack is not empty | ||
| /// ConditionalStackError.EmptyConditionalStack if the stack is empty |
There was a problem hiding this comment.
we know what pop return. You can remove this long comment
src/script/cond_stack.zig
Outdated
| /// Push a conditional value onto the stack | ||
| /// | ||
| /// Args: | ||
| /// value: The conditional value to push (typically 0, 1, or 2) |
There was a problem hiding this comment.
Can it be something else than 0, 1 or 2? You comment suggest it can
There was a problem hiding this comment.
Also can you define an enum for more readability
OpCondFalse = 0
OpCondTrue = 1
OpCondSkip = 2
There was a problem hiding this comment.
Can it be something else than 0, 1 or 2? You comment suggest it can
Reading other implementations it looks like it cannot. So please change your comment to not mean that it could
There was a problem hiding this comment.
And ofcourse use those OpCond enum in your tests rather than 1, 2, 3
| if (self.stack.items.len == 0) { | ||
| return ConditionalStackError.UnbalancedConditional; | ||
| } | ||
| _ = self.stack.pop(); |
There was a problem hiding this comment.
Don't you want to return the poped value?
| @@ -391,3 +391,27 @@ pub fn isUnnamedPushNDataOpcode(opcode: Opcode) ?u8 { | |||
| const opcodeByte = opcode.toBytes(); | |||
| return if (opcodeByte > 0x00 and opcodeByte <= 0x4b) opcodeByte else null; | |||
| } | |||
|
|
|||
| /// Conditional execution constants for script operations. | |||
| pub const OpConditional = enum(u8) { | |||
There was a problem hiding this comment.
Those are not Opcodes so I don't like the name.
What about "CondStackValue" ?
| /// | ||
| /// Returns: | ||
| /// Possible error if allocation fails | ||
| pub fn push(self: *Self, value: u8) !void { |
There was a problem hiding this comment.
So if values are strictly limited, change the signature to reflect that:
pub fn push(self: *Self, value: CondStackValue) !void
| const Self = @This(); | ||
|
|
||
| /// Dynamic array to store conditional values | ||
| stack: std.ArrayList(u8), |
There was a problem hiding this comment.
std.ArrayList(CondStackValue) is more restrictive and more expressive
|
@okhaimie-dev are you still working on this? |
I'll send a fix tomorrow, I have been traveling |
Implement conditional opcodes and wip to enhance script code structure
Added the following opcodes with corresponding tests:
opIfopNotIfopElseopEndIfOthers
opReservedopVerImprovements:
cond_stackfor managing conditional executionThis commit addresses issue #29