Levente Uzonyi wrote:
> On Tue, 16 Feb 2010, Juan Vuletich wrote:
>
>> Levente Uzonyi wrote:
>>> On Tue, 16 Feb 2010, Juan Vuletich wrote:
>>>
>>>> commits at source.squeak.org wrote:
>>>>> Levente Uzonyi uploaded a new version of System to project The Trunk:
>>>>> http://source.squeak.org/trunk/System-ul.253.mcz
>>>>>
>>>>> ==================== Summary ====================
>>>>>
>>>>> Name: System-ul.253
>>>>> Author: ul
>>>>> Time: 14 February 2010, 9:12:11.268 am
>>>>> UUID: 7aaed65b-d0e3-0040-8929-0eb0bf8ecf64
>>>>> Ancestors: System-ar.252
>>>>>
>>>>> - fix (again): Timer may be already nil in the #ensure: block in 
>>>>> MessageTally >> #spyEvery:on: and MessageTally >> #spyAllEvery:on:
>>>>>
>>>>
>>>> Hi Levente,
>>>>
>>>> How could that happen? Can you post a script to reproduce it?
>>>
>>> Well, the old way to reproduce this is fixed (press alt/cmd+. while 
>>> profiling) and that's great. But there's another one, it happens 
>>> when you're tallying something which tallies something else. I know 
>>> this shouldn't happen normally, but we have some tests which use 
>>> MessageTally (for no reason). If you press the "Run Profiled" button 
>>> in TestRunner, these tests will set Timer's value to nil and at the 
>>> end, you get a debugger.
>>>
>>>
>>> Levente
>>
>> Thanks, I see. Anyway, nested tallies are not working at all (and 
>> never worked). Your fix might give the impression that nested tallies 
>> are ok, when they are not. Perhaps it would be best to just raise an 
>> error.
>
> I agree that it's not the best solution and it shadows an issue. We 
> could just fix the tests and revert the code. But I think that 
> changing the value of a class variable from instance methods is a sign 
> of bad design. 

Indeed.

> Why is MessageTally using the class variable Timer instead of an 
> instance variable?

I really don't know. Perhaps to make it clear that nested tallies are 
not supported? BTW, I've just looked in Squeak 1.1, and it uses the 
Timer variable the same way. Hey, I even opened Apple Smalltalk-80, and 
it is the same too :).

> Levente
>
> P.S.:
> I added a new instance variable timer to MessageTally, replaced all 
> occurence of Timer to timer, removed the #ifNotNil: checks and 
> successfully profiled the tests. Is anything wrong with this approach?

I just tried the same. It seems to work. But  it seems whoever wrote 
MessageTally knew what he was doing. Using an instance variable would 
have been the obvious choice. But they decided to use a class variable. 
Mhhh. I looked at the old books and found nothing. Looking at Squeak 
1.1. and Smalltalk-80, I believe it was because at that time there was 
no #ensure: and they wanted to be sure to kill the process, as it won't 
stop if you got a walkback or pressed alt-.. So I guess your proposal is 
ok. But on the other hand, I see no value in supporting nested tallies, 
and your proposal would require some testing.

So, if you're willing to see how it works, and perhaps revert it if some 
problem arises, I'd say go ahead with it.

Cheers,
Juan Vuletich