harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexey Varlamov" <alexey.v.varla...@gmail.com>
Subject Re: [drlvm] harmony-1625 - hang on a sec...
Date Thu, 30 Nov 2006 07:46:55 GMT
2006/11/30, Alex Astapchuk <alex.astapchuk@gmail.com>:
> Geir Magnusson Jr. wrote:
> > I was looking over HARMONY-1625 and there are a few things I don't
> > understand...
> >
> > I see things like this :
> >
> >
> > const char *temp = m_env->properties->get(XBOOTCLASSPATH,
> > INTERNAL_PROPERTIES);
> >
> > ....
> >
> > STD_FREE((void*)temp);
> >
> >
> > 1) why is temp a const?  it's not - we'll free it later
>
> The 'const' and free() are orthogonal.
>
> The 'const' keywords here only means kind of hint for compiler, like
> 'user is not allowed to change the object'. This prevents things like
> temp[i] = xx at compile time.

Alex,
Thanks for noticing this.
Actually returned value should not be declared const, just because it
is a copy and user is allowed to do anything with it. Otherwise this
gives the wrong hint (both to human and compiler).
I suppose Geir basically meant just this, and this is exactly my
motivation to fix it.

>
> just my $.02.
>
> --
> Thanks,
>   Alex
>
>
> >
> > 2) I was going to suggest that having the user call free() is an awful
> > idea,, but I believe that you've actually added a recent change -
> > destroy_properties_string() or something to fix this - I assume that
> > this is something we can clean up.
> >
> > That said, I also see :
> >
> > +void Properties::destroy_value_string(const char* value) {
> > +    STD_FREE((void*)value);
> >
> > +void Properties::destroy_properties_keys(const char** keys) {
> > +    int i = 0;
> > +    while (keys[i] != NULL) {
> > +        if (keys[i]) {
> > +            STD_FREE((void*)keys[i]);
> > +        }
> > +        i++;
> > +    }
> > +    STD_FREE(keys);
> >
> > Which I think is slightly confusing.  I think that you need
> >
> >   destroy_string(char *)
> >   destroy_string(char **)
> >
> > and not distinguish between keys and values, unless they become classes
> > in their own right.
> >
> >
> > 3) looking at it, maybe the whole table number thing
> > ('INTERNAL_PROPERTIES') isn't a good idea when used in code.  Wouldn't
> > it be more elegant to have multiple properties in the environment such that
> >
> >     m_env->internalProperties->get(XBOOTCLASSPATH);
> >
> > or
> >     m_env->javaProperties->get("java.home");
> >
> >
> > kind of thing for readability?  it could map to the calls you have now...
> >
> > Then we could pass properties objects safely to code that should only
> > have one or the other...
> >
> >
> > geir
> >
>
>
>
>

Mime
View raw message