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 20:55:11 GMT

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....


> 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?


View raw message