Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More comparisons #55

Merged
merged 2 commits into from
Oct 9, 2019
Merged

More comparisons #55

merged 2 commits into from
Oct 9, 2019

Conversation

ltratt
Copy link
Member

@ltratt ltratt commented Oct 8, 2019

This PR adds more comparisons to yksom: reference equality (1dca2bc) and various comparisons for doubles (82d35c5). Notice that the former commit means that we diverge slightly from SOM's current semantics (see SOM-st/SOM#23) although I think it's fair to say that @smarr thinks the approach we're taking in this PR is the right one (and if I've got that wrong, hopefully he'll shout!).

ltratt added 2 commits October 8, 2019 15:03
This is roughly equivalent to the `is` operator in Python in that it checks
normal objects for pointer equality, but numbers are checked for value equality.
Note that SOM (or, at least, java-som) doesn't define/implement the latter cases
properly yet (see SOM-st/SOM#23) so this commit makes
us diverge slightly.

double10 = (
run = (
(1.0 = 0) println.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ==?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SOM is a little unusual (to our eyes) in this regadr. = means "check the values for equality". == means (roughly) "check the data pointer for equality" (though for integers this is overridden -- Python effectively does something similar).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the explanation.

(1.0 > 2) println.
(2.0 > 0) println.

(1.0 >= 1) println.
Copy link
Member

@ptersilie ptersilie Oct 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, so in SOM, 1.0 == 1 is false, 1.0 > 1 is false, but 1.0 >= 1 is true?

Copy link
Contributor

@smarr smarr Oct 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== is very different from the other two, and unrelated.

1.0 > 1 and 1 > 1 should yield both false, no? (minus issues with number representations for floating points, which hopefully do not matter for 1-like values, but will crop up with int-like values > 53-or-something bits).

And 1.0 >= 1 and 1 >= 1 should both be true in an ideal world (minus floating point issues)

Copy link
Member

@ptersilie ptersilie Oct 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks! With this comment and @ltratt's comment above, this makes a lot more sense now.

@ltratt
Copy link
Member Author

ltratt commented Oct 9, 2019

@smarr Just to confirm, do all the checks make sense to you (at least in yksom)?

@smarr
Copy link
Contributor

smarr commented Oct 9, 2019

@ltratt sorry, I didn't check in detail the true/false values. The test structure makes it hard for me to match up tested expression and expected value.
But, the general direction looked about right. Any difference, you'll notice soon enough when you try to run the SOM test suite :)

@ltratt
Copy link
Member Author

ltratt commented Oct 9, 2019

I compare Java SOM and yksom's output if that's any help. There are differences in lang_test/obj2.som, in particular.

@smarr
Copy link
Contributor

smarr commented Oct 9, 2019

obj2.som seems like it should be right in an ideal world.

@ltratt
Copy link
Member Author

ltratt commented Oct 9, 2019

@smarr OK, thanks! yksom, of course, aims to be an ideal world, a veritable nirvana, a stronghold of God's will on Earth. Except bugs, of course ;)

@ptersilie
Copy link
Member

Okay, I guess this is ready to merge then and I can tell bors the good news?

@ltratt
Copy link
Member Author

ltratt commented Oct 9, 2019

Yes please!

@ptersilie
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Oct 9, 2019
55: More comparisons r=ptersilie a=ltratt

This PR adds more comparisons to yksom: reference equality (1dca2bc) and various comparisons for doubles (82d35c5). Notice that the former commit means that we diverge slightly from SOM's current semantics (see SOM-st/SOM#23) although I think it's fair to say that @smarr thinks the approach we're taking in this PR is the right one (and if I've got that wrong, hopefully he'll shout!).

Co-authored-by: Laurence Tratt <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 9, 2019

Timed out

@vext01
Copy link
Member

vext01 commented Oct 9, 2019

Sorry, my CI changes yesterday broke this. I've applied the fix, but I just need a:

bors r+

bors bot added a commit that referenced this pull request Oct 9, 2019
55: More comparisons r=vext01 a=ltratt

This PR adds more comparisons to yksom: reference equality (1dca2bc) and various comparisons for doubles (82d35c5). Notice that the former commit means that we diverge slightly from SOM's current semantics (see SOM-st/SOM#23) although I think it's fair to say that @smarr thinks the approach we're taking in this PR is the right one (and if I've got that wrong, hopefully he'll shout!).

Co-authored-by: Laurence Tratt <[email protected]>
@ptersilie
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 9, 2019

Already running a review

@bors
Copy link
Contributor

bors bot commented Oct 9, 2019

Build succeeded

@bors bors bot merged commit 82d35c5 into softdevteam:master Oct 9, 2019
@ltratt ltratt deleted the comparisons branch October 9, 2019 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants