search

softdevteam / yksom

Other
8 stars 6 forks source link

Update the SOM core library #217

Closed smarr closed 1 year ago

smarr commented 1 year ago

This update the core library with various changes.

I am mostly curious whether things "just work", since you got a completely separate implementation. And, well, I don't think I can run yksom myself at the moment, since it seems to depend on custom Rust things.

Most notable:

Full list of commits: https://github.com/SOM-st/SOM/compare/26fa6abc4dba0d6406ae37aed1b11007c631b309...51d42a5e6606894d57c51f960e9bd57c1e274132

ltratt commented 1 year ago

What I'll do is give you the permission to bors try to your heart's content (you can also do bors r+ but please don't, at least not yet!).

bors delegate=smarr

bors[bot] commented 1 year ago

:v: smarr can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

ltratt commented 1 year ago

So you can now type:

bors try

and watch bors do its thing!

ltratt commented 1 year ago

Here's the current job (it'll take a while!):

http://bencher13.soft-dev.org:8010/#/builders/1/builds/1887

ltratt commented 1 year ago

Here's the relevant bits:

Failures: 1
    IntegerTest>>#testRaisedTo
        Expected 1267650600228229401496703205376 but was -1267650600228229401496703205376.
Test Suite: #StringTest
Tests passed: 16
------------------------------
Failures: 2
    StringTest>>#testEquality
        Assertion failed
    StringTest>>#testEquality
        Assertion failed
Test Suite: #SystemTest
Tests passed: 2
-----------------------

[I haven't investigated -- these could be yksom bugs!]

bors[bot] commented 1 year ago

try

Build failed:

smarr commented 1 year ago

Thanks.

IntegerTest>>#testRaisedTo
        Expected 1267650600228229401496703205376 but was -1267650600228229401496703205376.

This looks like an integer overflow check is missing for multiplication. Integer>>#raisedTo: is implemented with a simple loop: https://github.com/SOM-st/SOM/blob/master/Smalltalk/Integer.som#L51

    StringTest>>#testEquality
        Assertion failed
    StringTest>>#testEquality
        Assertion failed

This is very likely the new tests here:

https://github.com/SOM-st/SOM/commit/433b71f003be9e553838104b9f054f70541e0cea#diff-632d2583632ebc2d8753cbfb17fed906e205fb2c614c2869803cfcd611b228ecR38-R39

Smalltalk equality is not necessarily symmetric, and in the case of Strings and Symbols that becomes visible in these tests. So, a string is equal to a symbol when using =, because it only considers the string's content, but not the Symbol's identity.

ltratt commented 1 year ago

This looks like an integer overflow check is missing for multiplication.

Correct (and culprit found!). But isn't the test wrong in the sense that the number should scale up to a big int? 2**100 (according to Python) is 1267650600228229401496703205376. Shouldn't SOMs be transparently moving from small to big ints with multiplication?

smarr commented 1 year ago

This looks like an integer overflow check is missing for multiplication.

Correct (and culprit found!). But isn't the test wrong in the sense that the number should scale up to a big int? 2**100 (according to Python) is 1267650600228229401496703205376. Shouldn't SOMs be transparently moving from small to big ints with multiplication?

What exactly do you mean with the "test being wrong"? SOM doesn't expose the representation. So, on the code level, we don't see a distinction between small and big int.

Assuming that there's nothing wrong with the actual number in the test, I am not sure what you would expect to be different.

In SOM-java, I do for instance the following:

https://github.com/SOM-st/som-java/blob/master/src/som/vmobjects/SInteger.java#L155-L162 On multiplication, I catch the overflow, and indeed change the representation to be based on Java's BigInteger.

ltratt commented 1 year ago

I'm an idiot and misread the tests. Sorry!

smarr commented 1 year ago

Re the other bit, the equality, I actually just checked this in Squeak, and Squeak seems to only consider string equality even when #symbol = 'string' is compared, not just the other way around 'string' = #symbol. So, I think the test in SOM currently merely documents the status quo, and not necessarily what one would actually want.

So, that's just to say, I might change my mind on "what the right thing" is for the equality test of strings and symbols.

ltratt commented 1 year ago

It looks like the string equality tests work now I've fixed multiply! If/when https://github.com/softdevteam/yksom/pull/218 is merged, we can retry this PR -- I think it will "just work".

smarr commented 1 year ago

bors try

bors[bot] commented 1 year ago

try

Build failed:

ltratt commented 1 year ago

OK, so I do need to fix the stringy thingies.

ltratt commented 1 year ago

It looks like the string equality tests work now I've fixed multiply! If/when #218 is merged, we can retry this PR -- I think it will "just work".

Doh, I was running SOMSOM before. You can tell it's a while since I've looked at this repository...

ltratt commented 1 year ago

@smarr The test suite has string = symbol but not symbol = string. Are both 'a' = #a and #a = 'a' true?

ltratt commented 1 year ago

Ah, I see them now in SymbolTest.som.

ltratt commented 1 year ago

OK this is a bit baffling since all these tests succeed:

StringTest = TestCase (
  testEquality = (
        self assert: ('f' + 'o' + 'o') = #foo.
  )

  testEqualEqual = (
    | str1 |
    str1 := 'foo'.
    self assert: str1 == str1.
    self deny:   str1 == str1 asSymbol.
    self deny:   str1 == #foo.
  )
)

So somehow other SOMs are treating foo asSymbol as different than #foo? I feel like I'm missing out on something (I have made many SOM mistakes today!).

smarr commented 1 year ago

The tests use deny instead of assert, is that it?

I also added a todo to make up my mind about whether the current semantics are actually desirable: https://github.com/SOM-st/SOM/issues/105

Thoughts welcome.

ltratt commented 1 year ago

Possibly! I think I might have misread your "symmetric" comment. It seems that really = treats strings and symbols as totally interchangeable? FWIW, yksom currently says strings and symbols are not interchangeable. I don't feel strongly about that: to be honest, the differentiation of the two concepts feels fairly unimportant to me, but perhaps that's just my non-Smalltalk background.

smarr commented 1 year ago

I think in classic Smalltalk, the most important bit is that one can rely on == for symbols. Java did away with the distinction and instead introduced the concept of interned strings. I'll see what breaks when I make = symmetric... and get back to you then.

ltratt commented 1 year ago

I'll see what breaks when I make = symmetric... and get back to you then.

Any luck with that?

ltratt commented 1 year ago

Oops, I should have tagged @smarr on that last comment.

smarr commented 1 year ago

No, sorry, I got the notification, I just ignored you. Haven't managed to do anything on it yet...

ltratt commented 1 year ago

I won't take it personally :p

smarr commented 1 year ago

Seems like the change to make #= symmetric doesn't cause any issues. So, I'll probably go with it.

Relevant changes:

ltratt commented 1 year ago

I now have a change to yksom that works with the new definition of =. I'll close this PR, as I'll need to open a fresh one, with a (small) change to yksom + the new SOM commit (when you've pushed it to your master branch -- please let me know!).