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: [drlvm] harmony-1625 - hang on a sec...
Date Thu, 30 Nov 2006 05:37:26 GMT


Alexey Varlamov wrote:
> [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.

I guess I simply mean that as a user of this APi, I don't want to 
remember if it's a key or a value...

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

Ok - not sure - I need to figure out then what is internal and what isn't...
> 
>>
>> Anyway, not critical now.
> Yes. Ok, I'll play with this and see which is nicer.
> 
> [snip]

Mime
View raw message