|Anonymous | Login||04-03-2020 20:05 UTC|
|Main | My View | View Issues | Change Log | Docs|
|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|
|Summary||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].
depth := depth + (1 asLargerPowerOfTwo)
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.
|Attached Files||BMPReadWriter-nextPutImage.st [^] (2,912 bytes) 02-04-08 04:59|
(0011735 - 486 - 567 - 567 - 567 - 567 - 567)
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.
(0011736 - 289 - 343 - 343 - 343 - 343 - 343)
edited on: 02-04-08 05:23
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.
(0011740 - 443 - 612 - 612 - 612 - 612 - 612)
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].
permittedDepths := #(1 4 8 32 ).
( permittedDepths includes: depth )
ifFalse: [ depth := permittedDepths detect: [ :each | each > depth ] ].
(0011741 - 887 - 1037 - 1037 - 1037 - 1037 - 1037)
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
(0013361 - 60 - 60 - 212 - 212 - 212 - 212)
|Fixed in http://source.squeak.org/trunk/Graphics-nice.79.mcz [^]|
|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.