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

Fix for procedure Expo in module Reals #42

Open
kekcleader opened this issue Aug 29, 2016 · 3 comments
Open

Fix for procedure Expo in module Reals #42

kekcleader opened this issue Aug 29, 2016 · 3 comments

Comments

@kekcleader
Copy link

On my Debian 32 bit, procedure Reals.Expo (for REAL) didn't work right, whereas ExpoL (for LONGREAL) did work. As a result, Texts.WriteReal and Texts.WriteRealFix did not work (Texts.WriteLongReal did work).
I went ahead and fixed it (in src/library/v4/Reals.Mod):

  PROCEDURE Expo*(x: REAL): INTEGER;
    VAR i: INTEGER; l: LONGINT;
  BEGIN
    IF SIZE(INTEGER) = 4 THEN
      S.GET(S.ADR(x), i); 
      i := SHORT(ASH(i, -23) MOD 256)
    ELSIF SIZE(LONGINT) = 4 THEN
      S.GET(S.ADR(x), l); 
      i := SHORT(ASH(l, -23) MOD 256)
    ELSE HALT(98)
    END;
    RETURN i
  END Expo;

Before that it was just this:

  PROCEDURE Expo*(x: REAL): INTEGER;
  BEGIN
    RETURN SHORT(ASH(S.VAL(INTEGER, x), -23) MOD 256)
  END Expo;

And probably the issue had something to do with the incorrect work of SYSTEM.VAL for REAL to INTEGER conversion.

P.S. Not sure if SIZE(INTEGER) = 4 is required, but I added it for the future. The optimization removes the IF statement during compilation anyway.
I used the graphic over here for reference and this simple test program:

MODULE R;
IMPORT Out := Console, Reals, S := SYSTEM, Oberon, Texts;

PROCEDURE Expo(x: REAL): INTEGER;
  VAR i: INTEGER; l: LONGINT;
BEGIN
  IF SIZE(INTEGER) = 4 THEN
    S.GET(S.ADR(x), i);
    i := SHORT(ASH(i, -23) MOD 256)
  ELSIF SIZE(LONGINT) = 4 THEN
    S.GET(S.ADR(x), l);
    i := SHORT(ASH(l, -23) MOD 256)
  ELSE HALT(98)
  END;
  RETURN i
END Expo;

PROCEDURE Do;
VAR x: REAL; i: INTEGER; W: Texts.Writer;
BEGIN
  Texts.OpenWriter(W);
  x := 0.15625; (* Expo should return 124 = 01111100 *)
  REPEAT
    Texts.WriteLongReal(W, LONG(x), 25);
    Texts.Append(Oberon.Log, W.buf);
    i := Expo(x); Out.Int(i, 6); Out.Ln;
    x := x * 2
  UNTIL x > 10000
END Do;

BEGIN
  Do
END R.
@dcwbrown
Copy link
Contributor

There are loads of problems with real handling because of invalid size
assumptions in the code.

It happens by chance that I had already looked at this and committed the
following solution to the master branch 5 days ago:

 PROCEDURE Expo*(x: REAL): INTEGER;
 VAR i: INTEGER;
 BEGIN
 SYSTEM.GET(SYSTEM.ADR(x)+2, i);
 RETURN (i DIV 128) MOD 256
 END Expo;

Sorry - you must have just missed it :-(.

It's a simpler solution - would you mind trying it with your test?

There are so many issues with real across many, many library sources,
due to varying type sizes. Indeed I recently disabled a load of library
modules because they were not handling reals correctly.

I'm working on fixed integer types which will make coding the real
handling easier. It'll take me a while I'm afraid. But my goal is to
recode all the real implementations to be independent of
SHORTINT/INTEGER/LONGINT size.

Thanks -- Dave.

On 2016-08-29 16:30, Артур Ефимов wrote:

On my Debian 32 bit, procedure Reals.Expo (for REAL) didn't work right, whereas ExpoL (for LONGREAL) did work. As a result, Texts.WriteReal and Texts.WriteRealFix did not work (Texts.WriteLongReal did work).
I went ahead and fixed it (in src/library/v4/Reals.Mod):

PROCEDURE Expo*(x: REAL): INTEGER;
VAR i: INTEGER; l: LONGINT;
BEGIN
IF SIZE(INTEGER) = 4 THEN
S.GET(S.ADR(x), i);
i := SHORT(ASH(i, -23) MOD 256)
ELSIF SIZE(LONGINT) = 4 THEN
S.GET(S.ADR(x), l);
i := SHORT(ASH(l, -23) MOD 256)
ELSE HALT(98)
END;
RETURN i
END Expo;

Before that it was just this:

PROCEDURE Expo*(x: REAL): INTEGER;
BEGIN
RETURN SHORT(ASH(S.VAL(INTEGER, x), -23) MOD 256)
END Expo;

And probably the issue had something to do with the incorrect work of SYSTEM.VAL for REAL to INTEGER conversion.

P.S. Not sure if SIZE(INTEGER) = 4 is required, but I added it for the future. The optimization removes the IF statement during compilation anyway.
I used the graphic over here [1] for reference and this simple test program:

MODULE R;
IMPORT Out := Console, Reals, S := SYSTEM, Oberon, Texts;

PROCEDURE Expo(x: REAL): INTEGER;
VAR i: INTEGER; l: LONGINT;
BEGIN
IF SIZE(INTEGER) = 4 THEN
S.GET(S.ADR(x), i);
i := SHORT(ASH(i, -23) MOD 256)
ELSIF SIZE(LONGINT) = 4 THEN
S.GET(S.ADR(x), l);
i := SHORT(ASH(l, -23) MOD 256)
ELSE HALT(98)
END;
RETURN i
END Expo;

PROCEDURE Do;
VAR x: REAL; i: INTEGER; W: Texts.Writer;
BEGIN
Texts.OpenWriter(W);
x := 0.15625; (* Expo should return 124 = 01111100 *)
REPEAT
Texts.WriteLongReal(W, LONG(x), 25);
Texts.Append(Oberon.Log, W.buf);
i := Expo(x); Out.Int(i, 6); Out.Ln;
x := x * 2
UNTIL x > 10000
END Do;

BEGIN
Do
END R.

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub [2], or mute the thread [3].

Links:

[1]
https://ru.wikipedia.org/wiki/%D0%A7%D0%B8%D1%81%D0%BB%D0%BE_%D0%BE%D0%B4%D0%B8%D0%BD%D0%B0%D1%80%D0%BD%D0%BE%D0%B9_%D1%82%D0%BE%D1%87%D0%BD%D0%BE%D1%81%D1%82%D0%B8
[2] #42
[3]
https://github.com/notifications/unsubscribe-auth/ADChoDbohJmSwltnEcX6F-70SW3glcAkks5qkvstgaJpZM4Jvl96

@kekcleader
Copy link
Author

Hi Dave,

Yes, I must have missed the update. I've checked your solution with my test
and it works.

Sidenote: Just curious, what do you think about removing parentheses from
around i DIV 128 to make it:
RETURN i DIV 128 MOD 256 ?
Does it look more complex or simplier to you than with the parentheses?

Arthur

2016-08-29 20:30 GMT+03:00 David C W Brown notifications@github.com:

There are loads of problems with real handling because of invalid size
assumptions in the code.

It happens by chance that I had already looked at this and committed the
following solution to the master branch 5 days ago:

PROCEDURE Expo*(x: REAL): INTEGER;
VAR i: INTEGER;
BEGIN
SYSTEM.GET(SYSTEM.ADR(x)+2, i);
RETURN (i DIV 128) MOD 256
END Expo;

Sorry - you must have just missed it :-(.

It's a simpler solution - would you mind trying it with your test?

There are so many issues with real across many, many library sources,
due to varying type sizes. Indeed I recently disabled a load of library
modules because they were not handling reals correctly.

I'm working on fixed integer types which will make coding the real
handling easier. It'll take me a while I'm afraid. But my goal is to
recode all the real implementations to be independent of
SHORTINT/INTEGER/LONGINT size.

Thanks -- Dave.

On 2016-08-29 16:30, Артур Ефимов wrote:

On my Debian 32 bit, procedure Reals.Expo (for REAL) didn't work right,
whereas ExpoL (for LONGREAL) did work. As a result, Texts.WriteReal and
Texts.WriteRealFix did not work (Texts.WriteLongReal did work).
I went ahead and fixed it (in src/library/v4/Reals.Mod):

PROCEDURE Expo*(x: REAL): INTEGER;
VAR i: INTEGER; l: LONGINT;
BEGIN
IF SIZE(INTEGER) = 4 THEN
S.GET(S.ADR(x), i);
i := SHORT(ASH(i, -23) MOD 256)
ELSIF SIZE(LONGINT) = 4 THEN
S.GET(S.ADR(x), l);
i := SHORT(ASH(l, -23) MOD 256)
ELSE HALT(98)
END;
RETURN i
END Expo;

Before that it was just this:

PROCEDURE Expo*(x: REAL): INTEGER;
BEGIN
RETURN SHORT(ASH(S.VAL(INTEGER, x), -23) MOD 256)
END Expo;

And probably the issue had something to do with the incorrect work of
SYSTEM.VAL for REAL to INTEGER conversion.

P.S. Not sure if SIZE(INTEGER) = 4 is required, but I added it for the
future. The optimization removes the IF statement during compilation anyway.
I used the graphic over here [1] for reference and this simple test
program:

MODULE R;
IMPORT Out := Console, Reals, S := SYSTEM, Oberon, Texts;

PROCEDURE Expo(x: REAL): INTEGER;
VAR i: INTEGER; l: LONGINT;
BEGIN
IF SIZE(INTEGER) = 4 THEN
S.GET(S.ADR(x), i);
i := SHORT(ASH(i, -23) MOD 256)
ELSIF SIZE(LONGINT) = 4 THEN
S.GET(S.ADR(x), l);
i := SHORT(ASH(l, -23) MOD 256)
ELSE HALT(98)
END;
RETURN i
END Expo;

PROCEDURE Do;
VAR x: REAL; i: INTEGER; W: Texts.Writer;
BEGIN
Texts.OpenWriter(W);
x := 0.15625; (* Expo should return 124 = 01111100 *)
REPEAT
Texts.WriteLongReal(W, LONG(x), 25);
Texts.Append(Oberon.Log, W.buf);
i := Expo(x); Out.Int(i, 6); Out.Ln;
x := x * 2
UNTIL x > 10000
END Do;

BEGIN
Do
END R.

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub [2], or mute the thread
[3].

Links:

[1]
https://ru.wikipedia.org/wiki/%D0%A7%D0%B8%D1%81%D0%BB%D0%
BE_%D0%BE%D0%B4%D0%B8%D0%BD%D0%B0%D1%80%D0%BD%D0%BE%D0%B9_
%D1%82%D0%BE%D1%87%D0%BD%D0%BE%D1%81%D1%82%D0%B8
[2] #42
[3]
https://github.com/notifications/unsubscribe-auth/ADChoDbohJmSwltnEcX6F-
70SW3glcAkks5qkvstgaJpZM4Jvl96


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#42 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACQZMmvcN7O3r0AP-pSM1YKAHplR6Naaks5qkxdKgaJpZM4Jvl96
.

@dcwbrown
Copy link
Contributor

Good question. I feel more comfortable with the parentheses.

Using C/C++ I became accustomed to adding technically redundant parentheses to avoid finding yet again that C's rules had surprised me.

However I have just looked back to code I was writing in 1993 when I was happily using Modula-2 and yet to be adulterated by C, and found this:

CASE (hardware DIV 16) MOD 4 OF

So perhaps I shouldn't blame C.

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

No branches or pull requests

2 participants