Mantis Bugtracker
  

Viewing Issue Advanced Details Jump to Notes ] View Simple ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0006878 [Squeak] Graphics minor N/A 01-31-08 23:24 04-18-10 22:04
Reporter nicolas cellier View Status public  
Assigned To andreas
Priority normal Resolution fixed Platform
Status closed   OS
Projection none   OS Version
ETA none Fixed in Version trunk Product Version 3.10
  Product Build
Summary 0006878: [clean-up] BMPReadWriter>>#nextPutImage: bad smell use of asLargerPowerOfTwo
Description Enquire senders of asLargerPowerOfTwo and you will find this code:
    [#(1 4 8 32) includes: depth] whileFalse:[depth := depth + 1 asLargerPowerOfTwo].

That is:
    depth := depth + (1 asLargerPowerOfTwo)

That is:
    depth := depth + 1

Well, it works but smells bad...

Intention was good, but code would be much better with parenthesis:
  [#(1 4 8 32) includes: depth] whileFalse:[depth := (depth + 1) asLargerPowerOfTwo].
Steps To Reproduce
Additional Information
Yes, i agree, this will save very few cycles, and fall in the category cosmetic-minor-absurd-code-purification.

Thus i won't be able to produce a TestCase this time.
Attached Files  BMPReadWriter-nextPutImage.st [^] (2,912 bytes) 02-04-08 04:59

- Relationships

- Notes
(0011735 - 486 - 567 - 567 - 567 - 567 - 567)
wiz
02-04-08 04:59

Hi Nicolas

I think your fix would need to test depth 33 at the very least. :-)

The way this is written is not very robust. It relies on reasonable input not to infinitely recurse.*

What is wanted would be
... ifFalse: [ depth := #(1 4 8 32) detect: [ :each | each > depth ] ] .

Oui?

Yours in service and curiosity, -Jerome Peace

*I am sensitive to this, I have run into several infinite recursions in my bug hiking. Squeak code leads with its chin in this regard.
 
(0011736 - 289 - 343 - 343 - 343 - 343 - 343)
wiz
02-04-08 05:21
edited on: 02-04-08 05:23

Uploded BMPReadWriter-nextPutImage.st
with my suggested fix. I have not done ANY tests on it, yet.

I have not even looked to see if sunit has tests or if they run green.

I have thought of some to prove the fix but haven't had time to implement or test them tonite.

Cheers --Jer

 
(0011740 - 443 - 612 - 612 - 612 - 612 - 612)
nicolas cellier
02-04-08 20:33

Yes you are right. I did not patch depth>32 because creating such a form is by now impossible. But this is a hole in the future....

You could write even simpler (all cases treated in a single loop):

    depth := #(1 4 8 32 ) detect: [ :each | each >= depth].

instead of:

    permittedDepths := #(1 4 8 32 ).
    ( permittedDepths includes: depth )
        ifFalse: [ depth := permittedDepths detect: [ :each | each > depth ] ].
 
(0011741 - 887 - 1037 - 1037 - 1037 - 1037 - 1037)
wiz
02-04-08 21:18

Hi Nicolas

Ha. You are right it would be even easier and more elegant your way.

Ah. The benifits of pair programming thru a Mantis report.

Also note we have elimiated the only call (in 3.9) to #asLargerPowerOfTwo .


While trying to make a bad form I found like you did that forms couldn't be set that way. But other objects that respond to depth could (FlashMorph and TreeTurtle.)

My test would be something like

veryBadForm := FlashMorph new depth: 33 .

self shouldntTakeLong: [ self should: [ BMPReadWriter new nextPutImage: veryBadForm ] raise: Error ] .

That should distinguish before and after the fix.

where
#shouldntTakeLong:
is just the a simple form of a time limit test using a 1 millisecond parameter.

If you want to drive feel free to go ahead. Else I'll get back to this in due course.

Yours in curiosity and service, --Jerome Peace
 
(0013361 - 60 - 60 - 212 - 212 - 212 - 212)
nicolas cellier
10-06-09 19:51

Fixed in http://source.squeak.org/trunk/Graphics-nice.79.mcz [^]
 

- Issue History
Date Modified Username Field Change
01-31-08 23:24 nicolas cellier New Issue
01-31-08 23:24 nicolas cellier Status new => assigned
01-31-08 23:24 nicolas cellier Assigned To  => andreas
02-04-08 04:59 wiz Note Added: 0011735
02-04-08 04:59 wiz File Added: BMPReadWriter-nextPutImage.st
02-04-08 05:21 wiz Note Added: 0011736
02-04-08 05:23 wiz Note Edited: 0011736
02-04-08 20:33 nicolas cellier Note Added: 0011740
02-04-08 21:18 wiz Note Added: 0011741
10-06-09 19:50 nicolas cellier Issue Monitored: nicolas cellier
10-06-09 19:51 nicolas cellier Status assigned => resolved
10-06-09 19:51 nicolas cellier Fixed in Version  => trunk
10-06-09 19:51 nicolas cellier Resolution open => fixed
10-06-09 19:51 nicolas cellier Note Added: 0013361
04-18-10 22:04 andreas Status resolved => closed


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