Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0003712 [Squeak] Kernel major always 05-24-06 15:50 05-12-07 21:52
Reporter vj View Status public  
Assigned To
Priority normal Resolution open  
Status new   Product Version 3.8
Summary 0003712: Number readFrom: (1/2) storeString
Description Number can not read fractions. This is a fatal problem wnen using SIXX, for example.

Additional Information
Attached Files  Number class-readFrom.st [^] (1,077 bytes) 05-24-06 16:02
 FractionReadFromEnh-dm.1.cs [^] (1,790 bytes) 05-31-06 01:16
 FractionReadFromEnh-dm.2.cs [^] (1,868 bytes) 06-01-06 16:04
 FractionReadFromEnh-dm.3.cs [^] (1,888 bytes) 06-01-06 21:14
 FractionReadFromEnh-dm.4.cs [^] (1,816 bytes) 05-12-07 07:40

- Relationships

- Notes
(0005034 - 126 - 132 - 132 - 132 - 132 - 132)
vj
05-24-06 16:04

The atached bugfix is a simplae solution delegating the problem to Object which can read everything using Compiler>>evaluate:.
 
(0005132 - 620 - 698 - 698 - 698 - 698 - 698)
Duncan
05-30-06 16:30

I'm not sure if this is the right fix.

I tried your test expression in Dolphin, and it doesn't work there either.

There's an insightful comment in their implementation:

Implementation Note: The basic idea is to read the initial integer part and
then proceed to read a Float if a decimal point is found, a ScaledDecimal if a
$s is found, etc. Note that Fractions are not a literal type, as otherwise the
meaning of expressions such as '1+3/4' would not be consistent with the
normal language semantics (Smalltalk has no operator precedence).

Maybe the correct thing to do is not to use Number>>readFrom: ?
 
(0005133 - 573 - 753 - 753 - 753 - 753 - 753)
vj
05-30-06 17:08

Since the following expressions are true

    (1/2) storeString = '(1/2)'
    (1/2) isNumber = true

one could expect that

    Number readFrom '(1/2)'
    
had to work (as expected e.g. by SIXX).

It can be, of course, solved by fixing SIXX or anything to use Object readFrom:, but I think that the inability of Number to read its own storeString is a strange inconsistency.

The attached "fix" is a quick'n'dirty workaroud. The problem can be solved in a much better way, of course - read just what storeString says: '(', then integer, then '/', then integer, then ')'.
 
(0005143 - 85 - 97 - 97 - 97 - 97 - 97)
Duncan
05-31-06 01:16

With the help of Ken Causey on IRC, here's a nicer patch for this bug.

Thanks Ken!
 
(0005144 - 245 - 269 - 269 - 269 - 269 - 269)
KenCausey
05-31-06 01:19

vj's fix relies Object class>>readFrom: which uses the Compiler and therefore seems rather heavyweight to me.

Duncan's fix adds Fraction class>>readFrom: and tries to do this in much the way other Number subclasses do. I prefer Duncan's fix.
 
(0005147 - 43 - 43 - 43 - 43 - 43 - 43)
vj
05-31-06 11:16

Yes, that's it. I also prefer Duncan's fix.
 
(0005158 - 360 - 420 - 420 - 420 - 420 - 420)
Duncan
06-01-06 16:06

I noticed that with v1 of my changeset:

Fraction readFrom: '(1)' would result in a ZeroDivide error, but
Object readFrom: '(1)' answers '1' as expected.

So in v2 of the changeset, I added a check for zeros before returning the fraction.

Just a quick 2 line change:

numerator = 0 ifTrue: [^ denominator].
denominator = 0 ifTrue: [^ numerator].
 
(0005159 - 561 - 561 - 561 - 561 - 561 - 561)
KenCausey
06-01-06 17:09

Good catch. In general this sounds good but in the case where the numerator is equal to 0, '(0/8)' or something like that shouldn't the result be 0 (aka the numerator)? Or if you mean that you have read something like '(/23)' then shouldn't that be an error? I don't think you would want to get 23 back from that. Also, maybe you should instead check also for the '/' in addition to the leading parenthesis and if you don't get it just strip off the parentheses and hand it back to Number. That way if someone does '(23.3784)' it will be handled correctly.
 
(0005160 - 672 - 672 - 672 - 672 - 672 - 672)
KenCausey
06-01-06 17:30

Actually, I guess what you really need to check for is that you have a number, a /, and another number with the second number not being 0, this of course after the detection of parentheses has handed off handling to Fraction. If you have the slash but not both numbers (and a literal 0 should count as a number) then I think it should be an error. If there is no slash then it should be handled back to Number. If the denominator = 0 then that's an error (division by zero). I believe that covers all the 'degenerate cases'. Everthing else, including the numerator being zero should be OK (of course for performance reasons you might shortcut the numerator = 0 case).
 
(0005174 - 373 - 464 - 464 - 464 - 464 - 464)
Duncan
06-02-06 01:42

rev3 does the following checks:

"The first char after the opening paren must be a digit"
aStream peek isDigit ifFalse: [^ self error: 'This is not well-formed.'].

numerator = 0 ifTrue: [^ 0].

This seems to be consistent with Object>>readFrom:.

'(123)' => 123 with both
'(1/)' => ZeroDivide with Fraction, and Syntax error with Object
'(0/2)' => 0 with both
 
(0010708 - 111 - 167 - 167 - 167 - 167 - 167)
nicolas cellier
05-11-07 23:58

Well, it seems you forgot this one:

    Fraction readFrom: (-3/4) storeString.

waiting for rev4

cheers
 
(0010709 - 222 - 262 - 262 - 262 - 262 - 262)
Duncan
05-12-07 07:40

I think the following check is unncessary:

"The first char after the opening paren must be a digit"
aStream peek isDigit ifFalse: [^ self error: 'This is not well-formed.'].

Am I missing any cases by removing this?
 
(0010711 - 708 - 792 - 792 - 904 - 904 - 904)
nicolas cellier
05-12-07 21:52

Hi Duncan.
Version 4 is now perfect for well formed Fraction as produced by storeString.
Of course, not so perfect for non well formed...

What we learn here is:
- when we introduce error tests we often introduce errors (of course, i count myself in we)...
- it is more difficult to handle error cases than regular cases
- this is because error can happen at each parsing stage

That's why i proposed to read numbers with a NumberParser better equipped with error handling see
- SqNumberParser class
- bug 0003512
- 'Fun with Number readFrom: What should we do ?' in squeak-dev for lengthy arguments.

A simple exercize would be to port Fraction reading in some SqNumberParsser specific method...
 

- Issue History
Date Modified Username Field Change
05-24-06 15:50 vj New Issue
05-24-06 16:02 vj File Added: Number class-readFrom.st
05-24-06 16:04 vj Note Added: 0005034
05-28-06 21:03 vj Issue Monitored: vj
05-30-06 16:30 Duncan Note Added: 0005132
05-30-06 17:08 vj Note Added: 0005133
05-31-06 01:16 Duncan Note Added: 0005143
05-31-06 01:16 Duncan File Added: FractionReadFromEnh-dm.1.cs
05-31-06 01:19 KenCausey Note Added: 0005144
05-31-06 11:16 vj Note Added: 0005147
06-01-06 16:04 Duncan File Added: FractionReadFromEnh-dm.2.cs
06-01-06 16:06 Duncan Note Added: 0005158
06-01-06 17:09 KenCausey Note Added: 0005159
06-01-06 17:30 KenCausey Note Added: 0005160
06-01-06 21:14 Duncan File Added: FractionReadFromEnh-dm.3.cs
06-02-06 01:42 Duncan Note Added: 0005174
05-11-07 23:58 nicolas cellier Note Added: 0010708
05-12-07 07:40 Duncan File Added: FractionReadFromEnh-dm.4.cs
05-12-07 07:40 Duncan Note Added: 0010709
05-12-07 21:52 nicolas cellier Note Added: 0010711


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
95 total queries executed.
54 unique queries executed.
Powered by Mantis Bugtracker