Mantis - Squeak
Viewing Issue Advanced Details
6878 Graphics minor N/A 01-31-08 23:24 04-18-10 22:04
nicolas cellier  
closed 3.10  
none trunk  
0006878: [clean-up] BMPReadWriter>>#nextPutImage: bad smell use of asLargerPowerOfTwo
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].

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. [^] (2,912 bytes) 02-04-08 04:59

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 ] ] .


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.
02-04-08 05:21   
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

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 ] ].
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.

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
nicolas cellier   
10-06-09 19:51   
Fixed in [^]