|Anonymous | Login||12-17-2018 14:00 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|
|0006584||[Squeak] System||major||always||08-01-07 17:24||04-18-10 22:05|
|Assigned To||nicolas cellier|
|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.|
|Attached Files||SymbolAllintancesFix.1.cs [^] (8,584 bytes) 08-01-07 17:24|
(0010954 - 452 - 548 - 548 - 548 - 548 - 548)
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)
|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)
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)
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.
Yours in curiosity and service, --Jerome Peace
(0011205 - 799 - 859 - 859 - 859 - 859 - 859)
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)
Installer mantis bug: 6584 fix:'SymbolAllintancesFix.1.cs'.
(0013309 - 169 - 175 - 325 - 325 - 325 - 325)
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.
|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.