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 21:04:01 GMT
Geir Magnusson Jr. wrote:
> 
> 
> Gregory Shimansky wrote:
>> Geir Magnusson Jr. wrote:
>>>
>>>
>>> Gregory Shimansky wrote:
>>>> Geir Magnusson Jr. wrote:
>>>>>
>>>>>
>>>>> Gregory Shimansky wrote:
>>>
>>>>>>
>>>>>> 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 :)
>>>
>>> No - you're missing my point completely.  it isn't about the commit 
>>> comments, but rather the source.  Because of the unorthodox name of 
>>> the class and lack of any comment *in the code* it's unclear to a 
>>> reader what's going on.
>>>
>>> It was certainly unclear to me.
>>>
>>> The class is a cute trick, and I can see how it makes life easier for 
>>> maintenance, but we also have to consider that this code is being 
>>> written for collective ownership and maintenance, so the more 
>>> explicit and clear we can be, the better off the project is.
>>>
>>> Always write code assuming that you win the lottery and retire 
>>> tomorrow, so make it clear enough that there are few questions.
>>
>> If you grep the source you'll find that Suspend.*Checker classes are 
>> used a lot. Do you suggest to put a comment before every use? It is 
>> just the first time you've seen this call, so it was not clear to you. 
>> Simply finding the class definition would've answered all your 
>> questions. It is a normal thing to do when you see that some function 
>> is called, and don't understand what it does.
> 
> Why use names at all then?  Why not "A", "B", "C" for your 
> classnames...? (and "a", "b" "c" for your function names...)
> 
> Then if someone points out that it made the code not obvious to the 
> reader, we can just tell them to use grep and look it up....
> 
> :D

I didn't think that the name is so unclear, and the main problem was 
that it is unclear why define a local variable which is not used.

>>
>> I understand the need to document the code and put comments into it. I 
>> also think that vmcore/include/suspend_checker.h could have some 
>> doxyden docs. But putting identical comments like "check the that 
>> suspend is enabled/disabled when entering and leaving this function" 
>> all around the code, seems to be too excessive to me.
>>
> 
> How about renaming it?  "EnableDisableAutoAssert" or something?

Ok I'll put it on my todo list.

-- 
Gregory


Mime
View raw message