harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Evgueni Brevnov" <evgueni.brev...@gmail.com>
Subject Re: svn commit: r479181 - /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
Date Tue, 28 Nov 2006 05:41:34 GMT
I just noted this discussion. I support an idea of checking thread's
suspend enabled/disabled state automatically. It seems to be useful
and convenient approach to me. To be honest I don't see huge
difference between two names SuspendDisabledChecker &
EnableDisableAutoAssert. Though the last one sounds better.

Thanks
Evgueni

On 11/28/06, Gregory Shimansky <gshimansky@gmail.com> wrote:
> 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