SOM-st / SOM

SOM - Simple Object Machine
http://som-st.github.io/
Other
64 stars 12 forks source link

Fixed various issues related to `Vector` indices #90

Closed Hirevo closed 2 years ago

Hirevo commented 2 years ago

As I was continuing to use the Vector class more and more intensely when solving the Advent of Code puzzles, I discovered some more potential issues about how Vector behaves after a call to Vector>>#removeFirst, specifically around the indices that it uses and/or allows.

After a single call to Vector>>#removeFirst for example, the current behaviour is that:

This PR fixes these issues and makes it so that the user always uses and observes indices starting from 1 onwards (the vector takes care of making it relative to first when indexing into storage).

And another unrelated bug fix that I have included here is that an off-by-one access past the end of the vector simply always returned nil, instead of calling Object>>#error: (which is invoked starting from off-by-two errors).

Here is some code that shows the bugs:

MyVectorTest = (
    | vec |

    vec := 42, 65, 34, 73, 24, 20.

    vec removeFirst.       " <- this correctly returned `42` "
    vec removeFirst.       " <- this correctly returned `65` "

    vec asArray.           " <- BUG: this returned `#(nil nil 34 73)` instead of `#(34 73 24 20)` "

    vec doIndexes: [ :idx | idx print ]. '' println.   " <- BUG: this printed `3456` instead of `1234` "

    (vec at: 2) println.  " <- BUG: this crashed the program with an `index 2 out-of-bounds of [3..7]` message "
    vec at: 1 put: 99.    " <- BUG: this also crashed the program with an `index 1 out-of-bounds of [3..7]` message "

    " Another slight bug about indices, but needing to call `Vector>>#removeFirst:` beforehand to trigger it "
    Vector new at: 1.   " <- BUG: this returned nil, instead of an raising an off-by-one error, although it did if we used `2` instead "
)
Hirevo commented 2 years ago

These changes do not break anything in the test suite but I wonder whether any of these is actually a breaking change for some other SOM code somewhere that I am not aware of.

Also, maybe these behaviours are intended to be that way, in which case, I have no issues dropping the PR.

smarr commented 2 years ago

Absence of test failure is probably not a good indicator, since coverage isn't great. I'll probably not get to any of these PRs this week anymore. Sorry. Will need to take a closer look to be sure what's going on.

smarr commented 2 years ago

Also, I should say: thanks a lot for all this stuff. It's greatly appreciated!

smarr commented 2 years ago

@Hirevo could you either pull the tests from here: https://github.com/Hirevo/SOM/compare/fix/vector-indices...smarr:fix/vector-indices?expand=1, or tick the "maintainers can push" box, I think on the right of the PR somewhere.

This adds a small clarification in the code, as well as a bunch of unit tests.

Thanks!

Hirevo commented 2 years ago

Th UI seems to tell me that pushes from maintainers are already allowed (the checkbox on the right was already checked).

In any case, I think I may be able to cherry-pick your commits into this branch.

Hirevo commented 2 years ago

Cherry-picking the commits seems to have done the trick.

smarr commented 2 years ago

Thank you!