harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Geir Magnusson Jr." <g...@pobox.com>
Subject Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Date Mon, 27 Nov 2006 15:37:39 GMT


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


> 
>>>
>>>> gshimansky@apache.org wrote:
>>>>> Author: gshimansky
>>>>> Date: Sat Nov 25 11:59:38 2006
>>>>> New Revision: 479181
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=479181
>>>>> Log:
>>>>> Fixed 32-bitness in classloader tracing. Added assertion before 
>>>>> using a
>>>>> raw heap object pointer
>>>>>
>>>>>
>>>>> Modified:
>>>>>    
>>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp

>>>>>
>>>>>
>>>>> Modified:
>>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp

>>>>>
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/vmcore/src/c

>>>>>
>>>>> lass_support/classloader.cpp?view=diff&rev=479181&r1=479180&r2=479181
>>>>> =========================================================================

>>>>>
>>>>> ===== ---
>>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp

>>>>>
>>>>> (original) +++
>>>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp

>>>>>
>>>>> Sat Nov 25 11:59:38 2006 @@ -589,11 +589,13 @@
>>>>>
>>>>>  ClassLoader* ClassLoader::AddClassLoader( ManagedObject* loader )
>>>>>  {
>>>>> +    SuspendDisabledChecker sdc;
>>>>> +
>>>>>      LMAutoUnlock aulock( &(ClassLoader::m_tableLock) );
>>>>>      ClassLoader* cl = new UserDefinedClassLoader();
>>>>>      TRACE2("classloader.unloading.add", "Adding class loader "
>>>>>          << cl << " (" << loader << " : "
>>>>> -        << 
>>>>> ((VTable*)(*(unsigned**)(loader)))->clss->get_name()->bytes
>>>>> << ")"); +        << loader->vt()->clss->get_name()->bytes
<< ")");
>>>>>      cl->Initialize( loader );
>>>>>      if( m_capacity <= m_nextEntry )
>>>>>          ReallocateTable( m_capacity?(2*m_capacity):32 );
>>>
>>
> 
> 

Mime
View raw message