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.

Sign up for GitHub

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

Jump to bottom

Review #asInteger #4701

Closed
astares opened this issue Sep 23, 2019 · 6 comments · Fixed by #4714
Closed

Review #asInteger #4701

astares opened this issue Sep 23, 2019 · 6 comments · Fixed by #4714

Comments

@astares
Copy link
Member

astares commented Sep 23, 2019

As reported by Frank Lesser [LSW] on Discord today in channel #development:

(1/2) asInteger = 0

caused by Number >>#asInteger - not conforming to ANSI
kasperosterbye added a commit to kasperosterbye/pharo that referenced this issue Sep 24, 2019
@kasperosterbye
Fixes pharo-project#4701
57dc57d 
Ansi says to use rounded rather than truncated :
```
5.6.2.16.Message: asInteger Synopsis
Answer an integer approximating the receiver.
Definition: <number>
Answer the result of sending #rounded to the receiver.
````

Changed to `rounded`, ran all the tests that seemed relevant, and added a testcase for the issue at hand.
kasperosterbye added a commit to kasperosterbye/pharo that referenced this issue Sep 24, 2019
@kasperosterbye
Fixes pharo-project#4701
29b5711 
Changed the comment to match the ansi standard
@kasperosterbye kasperosterbye mentioned this issue Sep 24, 2019
Merged
kasperosterbye added a commit to kasperosterbye/pharo that referenced this issue Sep 25, 2019
@kasperosterbye
Fixes pharo-project#4701
92e3d4d 
The implementation of asInteger doing truncation instead of the ansi way of doing rounded is taken over from Squeak. It seems like the morphic system is depending on asInteger doing truncation instead of rounding. I added a comment in asInteger to warn about the fact that we are not ansi compliant.
@nicolas-cellier-aka-nice
Copy link
Contributor

I just wonder the rationale for answering rounded (nearest int), is it really in the final version?
Which dialect is conforming to this?
Not st80, so not surprisingly not VW/Squeak/Pharo...

@frank-lesser
Copy link

I dont want to participate on a discussion weather asInteger should truncate or round.
ANSI defines rounded and Stephane Ducasse as the Pharo autority decided to keep the truncated version with
a clear comment in the implementation.
How I myself fixed senders of asInteger is to use either rounded or truncated. In Pharo you have for instance
BitBlt >>#roundVariables, which IMO gets improved when using rounded instead of truncated.
And yes there exists Smalltalk dialects which conforms to the ANSI standard including my own.

@Ducasse
Copy link
Member

Ducasse commented Sep 26, 2019

Hello nicolas
what do you think?

@frank-lesser I do not understand your point then.
we have to balance risk gain ratio when we change something that is in the core. Remember that this definition works since 1998.
And ANSI is not an objective for us. If we are compatible then it is good else we do not care. Remember Pharo is not Smalltalk. Our objective is not to build Smalltalk. Our objective is to build Pharo :)
Now if you see places that can take advantage of rounded instead of truncated, open bug entries and we will check them.

About roundVariables, I see that it is used in the primitive so we will have check.

@frank-lesser
Copy link

frank-lesser commented Sep 26, 2019 •
edited
Loading

my point was to accept the decision with the clear comment in the implementation, and to review senders of asInteger ( example roundVariables ) in order to improve the quality.
But if you dont care about ANSI, even the comment is unneeded.

@nicolas-cellier-aka-nice
Copy link
Contributor

nicolas-cellier-aka-nice commented Sep 26, 2019 •
edited
Loading

Even if ANSI conformance is not a primary goal, the knowledge of conformance still has a value.
For now, I would say that asInteger behaviour is a legacy from st80, but we have no clue of the driving rationale, neither in ANSI, nor in st80...
ISO/IEC 10967-2 does not seem to have any rule covering a generic convert float->int
§5.4.2 of draft http://www.open-std.org/JTC1/SC22/WG11/docs/iso10967-2_2001_e.pdf only covers explicit rounding direction...
Some other dialects like Dolphin and gst are conformant and use rounded...

I would advise using explicit direction whenever possible.

@Ducasse
Copy link
Member

Ducasse commented Sep 26, 2019

Frank Ok I got your point. Thanks for the clarification. We should at least document the difference in a comment. This has a value.

@weekly-digest weekly-digest bot mentioned this issue Sep 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

4701-Review-asInteger- kasperosterbye/pharo
4 participants
@Ducasse @nicolas-cellier-aka-nice @astares @frank-lesser