fix: return error if branch not found in child order #364
Conversation
go/proof.go
Outdated
| } | ||
| } | ||
| panic(fmt.Errorf("branch %d not found in order %v", branch, order)) | ||
|
|
||
| return 0, fmt.Errorf("branch %d not found in order %v", branch, order) |
There was a problem hiding this comment.
This is basically the change needed. The rest of changes are for bubbling the error up.
There was a problem hiding this comment.
Alternatively I could have just swallowed the error in the call sites of getPosition and return in those places the default value (false, and 0s). I thought of bubbling the error up in case we ever do #29.
go/proof.go
Outdated
| if branch < 0 || int(branch) >= len(order) { | ||
| panic(fmt.Errorf("invalid branch: %d", branch)) | ||
| return 0, fmt.Errorf("invalid branch: %d", branch) |
There was a problem hiding this comment.
I also converted this one into a regular error instead of panicking.
| idx, err := orderFromPadding(spec, op) | ||
| if err != nil { | ||
| return false | ||
| return false, err |
There was a problem hiding this comment.
Here an error that previously was swallowed can now be returned.
go/proof.go
Outdated
| idx, err := orderFromPadding(spec, op) | ||
| if err != nil { | ||
| return false | ||
| return false, nil |
There was a problem hiding this comment.
Another one that previously was swallowed.
go/proof.go
Outdated
| leftBranchesAreEmpty, err := leftBranchesAreEmpty(spec, step) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if !hasPadding(step, minPrefix, maxPrefix, suffix) && !leftBranchesAreEmpty { | ||
| return false, nil |
There was a problem hiding this comment.
todo: did this short circuit before if hasPadding returned true? (ie would that have an effect if we now call left branches first?
There was a problem hiding this comment.
I decided to change the code such that it would execute with the exact same code trace as before:
@crodriguezvega changed it to:
leftBranchesAreEmpty, err := leftBranchesAreEmpty(spec, step)
if err != nil {
return false, err
}
if !hasPadding(step, minPrefix, maxPrefix, suffix) && !leftBranchesAreEmpty {
return false, nil
}I changed it to:
if !hasPadding(step, minPrefix, maxPrefix, suffix) {
leftBranchesAreEmpty, err := leftBranchesAreEmpty(spec, step)
if err != nil {
return false, err
}
if !leftBranchesAreEmpty {
return false, nil
}
}Because golang has short circuiting, !hasPadding would have previously resulted in rightBranchesAreEmpty() not being called, but in the code @crodriguezvega refactored, it would have been called on every execution. I tried to look at the correctness of such change, if rightBranchAreEmpty would function regardless of the determined padding, but it was too exhausting of a mental exercise to justify the minor differences in readability, so I opt'd to leave execution as it was before
No description provided.