Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0006584 [Squeak] System major always 08-01-07 17:24 04-18-10 22:05
Reporter pmm View Status public  
Assigned To nicolas cellier
Priority high Resolution fixed  
Status closed   Product Version 3.8
Summary 0006584: Symbol allInstances doesn't work anymore
Description Since the WideString refactoring Symbol is an abstract class like String. It doesn't have any instances anymore. Yet there are still senders of it in ImageSegment related code. They store it into a temprory variable to prevent them from beeing garbage collected. This is clearly broken now. The correct way to fix this is to send Symbol allSymbols.
Additional Information
Attached Files  SymbolAllintancesFix.1.cs [^] (8,584 bytes) 08-01-07 17:24

- Relationships

- Notes
(0010954 - 452 - 548 - 548 - 548 - 548 - 548)
wiz
08-02-07 01:50

Can you provide sunit tests with it.

At least one simple one that fails before the patch and passes after

a method that does

self shouldnt: [ something that fails before the patch] raise: Error.

would at least flag if the patch is missing.

If you can think of other ways to check the pieces even better.

Your in service, --Jerome Peace

Note: I didn't test the code.

Just glanced at it to notice that it is several methods long.
 
(0010957 - 153 - 153 - 153 - 153 - 153 - 153)
pmm
08-02-07 11:51

The patch is not several methods long. The methods that are patched (one line is changed in each) are several methods long. So the patch is only 3 lines.
 
(0011191 - 340 - 358 - 358 - 358 - 358 - 358)
nicolas cellier
09-24-07 22:18

I guaranty this patch as valid and non-regressive.
It seems to me like a non-sense to write long-long-crooked tests for something so obvious and already discussed in Squeak-dev.

If you wait for a test of ImageSegment to appear (that would make sense), you might let these bugs rot in the image for several years. Please include in 3.10!
 
(0011201 - 2288 - 2483 - 2483 - 2483 - 2483 - 2483)
wiz
09-25-07 05:24

Hi nicolas, hi pmm

I track bugs. The patterns I see are that a patched bug usually receives other patches. Often done by several code cooks.

Tests prevent reversion of fixes by demonstrating a difference. And in that sense they make me feel more comfortable about the future of the image.

Nicolas what made you sure this patch is valid and non-regressive? What stops you from putting that in a simple bug test?

>It seems to me like a non-sense to write long-long-crooked tests for something so obvious and already discussed in Squeak-dev.

Why would the test be long-long-crooked? Testing any simple aspect of the fix that demonstrates the difference is a sufficeint first approximation of what is needed. I read your comments to mean that the methods in question don't have much test coverage at the moment and that the task of giving them some would be onerous, especially to someone who just wants the bug fixed.

I agree, it would be an imposition to ask for repairs that should belong to the code authors. I just ask that patchers take the responsibility of proving their own code is
1) necessary because there is a test that fails if the the code is not included.
2) sufficient because the test passes when the patch is in place.

Even superficial tests with the above two properties would be benificial to the integrity of squeak.

I take your word, Nicolas, at great value. You are particular about things and have high standards.If you vouch for the code I accept that the code is good code.

The tests are to insure it remains so.

I track and fix bugs, I don't harvest them. My requests are just that, requests. They are not demands or preconditions for acceptance.

I have learned that the first rule of trouble shooting is first hand information. Tests are important because they provide that and point to the problem area when they fail.

I believe your word. But it only applies to the here and now. Not for what the code might be patched to in the future. Or what else might be added to squeak that would interact with it.

I figure if I can get the good folks to write tests for their good code that gives us all a place to stand when we ask the same of the stumblers.

Cheers,

Yours in curiosity and service, --Jerome Peace
 
(0011205 - 799 - 859 - 859 - 859 - 859 - 859)
nicolas cellier
09-25-07 08:21

Hi Jerome,
Agree with your general comments, but every rule has exceptions.
I cannot tell if new code is really good, I can only tell it is obviously better than older because it fixes an obvious bug.

Old code call (Symbol allInstances) which is now a NON SENSE because Symbol is now an Abstract class. (Symbol allSymbols) is a well-known-far-better solution.

Maybe the simple test could be to scan source code and check that such a construct is not used, based on Abstract Syntax Tree Analysis.
Should we really test all possible such ill-constructions?

I believe tests should rather concentrate on asserting the functionality and the intention of the messages.
So this lead to the long-long way, that is to test ImageSegment, which very few can understand I guess. So it won't happen.
 
(0011729 - 95 - 143 - 143 - 143 - 143 - 143)
nicolas cellier
02-01-08 22:58

"fix begin"
Installer mantis bug: 6584 fix:'SymbolAllintancesFix.1.cs'.
"fix test"
"fix end"
 
(0013309 - 169 - 175 - 325 - 325 - 325 - 325)
nicolas cellier
09-16-09 19:59

Fixed in http://source.squeak.org/trunk/System-nice.148.mcz [^]
Trunk fix is NOT SymbolAllintancesFix.1.cs above, because eem applied some other changes related to closure.
 

- Issue History
Date Modified Username Field Change
08-01-07 17:24 pmm New Issue
08-01-07 17:24 pmm File Added: SymbolAllintancesFix.1.cs
08-02-07 01:50 wiz Note Added: 0010954
08-02-07 11:51 pmm Note Added: 0010957
09-24-07 22:18 nicolas cellier Note Added: 0011191
09-25-07 05:24 wiz Note Added: 0011201
09-25-07 08:21 nicolas cellier Note Added: 0011205
02-01-08 22:58 nicolas cellier Note Added: 0011729
01-10-09 02:00 Keith_Hodges Status new => pending
09-16-09 19:59 nicolas cellier Status pending => resolved
09-16-09 19:59 nicolas cellier Fixed in Version  => trunk
09-16-09 19:59 nicolas cellier Resolution open => fixed
09-16-09 19:59 nicolas cellier Assigned To  => nicolas cellier
09-16-09 19:59 nicolas cellier Note Added: 0013309
04-18-10 22:05 andreas Status resolved => closed


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