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 04:02:34 GMT
[snip]
> >>
> >> 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.
> >
> > Maybe worth it, but only for vmcore internal  API. The exported
> > interfaces are pure-C thus no overloading is allowed.
> > And I'm inclined to replace "destroy" with more conventional "free".
>
> Ok - but there should be no need to have API calls for "keys" and
> "values", IMO.
I'm confused now. Do you mean API naming only? Or suggest to have a
single destroy() for all?
For the latter, it should be possible to alloc single memory block for
storing keys array + key strings.
>
> >
> >>
> >>
> >> 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...
> >>
> > Agreed, thought of this, just staggerred by the amount of work to
> > rewrite the patch :(
> > And again, this would differentiate internal and exported APIs further
> > - it was nice to have them looking similar.
>
> eh - they don't - there's that table number value...
Huh? Didn't you suggest to get rid of the table number in interanl API?

>
> Anyway, not critical now.
Yes. Ok, I'll play with this and see which is nicer.

[snip]

Mime
View raw message