harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gregory Shimansky <gshiman...@gmail.com>
Subject Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Date Mon, 27 Nov 2006 19:51:31 GMT
Geir Magnusson Jr. wrote:
> 
> 
> Gregory Shimansky wrote:
>> Geir Magnusson Jr. wrote:
>>>
>>>
>>> Gregory Shimansky wrote:
>>>> On Sunday 26 November 2006 03:30 Geir Magnusson Jr. wrote:
>>>>> 1) Why add the SuspendDisabledChecker if not using it?
>>>>>
>>>>> 2) exactly where did you add the assertion?  :)
>>>>
>>>> It is a hidden assertion class :)
>>>
>>> Oh, come on.  An assertion as a side effect?
>>
>> No, assertion is one and the only purpose of this class. Isn't word 
>> Checker in class name is not descriptive enough.
> 
> No, clearly not :)
> 
>>
>>>>
>>>> Look at the file vm/vmcore/include/suspend_checker.h. There are 2 
>>>> classes SuspendEnabledChecker and SuspendDisabledChecker. When 
>>>> declared, such variable in constructor checks for suspend status, in 
>>>> destructor it checks that the status is the same. It is often 
>>>> convenient to write just this one line to make sure that all returns 
>>>> from the function have the same suspend status because local 
>>>> variable destructor is executed on every function exit.
>>>>
>>>> In release, when assert is a noop, these constructors/destructors 
>>>> are optimized into noop as well.
>>>>
>>>> I saw that this function uses raw ManagedObject pointers. This is 
>>>> dangerous in case when suspend is enabled (equal to GC being 
>>>> enabled) as the object may be moved at any time. So I decided to add 
>>>> this assertion. If it fails some time it will signal that this 
>>>> function is called in a wrong unsafe mode.
>>>
>>> Ok - but don't you think that the assert() would be clearer and just 
>>> as efficient?
>>
>> In this function maybe, but in other places where it is used, the fact 
>> that all return sites in the function are covered by this one line is 
>> very convenient. Also if function changes, someone may forget to put 
>> assertion before some return statement.
> 
> I'll admit, it is darn convenient.
> 
>>
>> In VM code SuspendChecker usage is clear enough, it is quite widely 
>> used, although there is a lot of code which wasn't converted since 
>> checker classes were introduced. New code uses them, old code uses 
>> assertions.
> 
> It's not clear to a casual reader.  I don't know if a rename is in 
> order, or the addition of a comment when used, but clearly to claim that 
> you added an "assert" and then use a class called "Checker" is far from 
> transparent.
> 
> geir

I'll try to make more descriptive commit comments :)


-- 
Gregory


Mime
View raw message