Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] 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  
Status closed   Product Version 3.10
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].
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