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 03:31:50 GMT


Alexey Varlamov wrote:
> 2006/11/30, Geir Magnusson Jr. <geir@pobox.com>:
>> 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
> 
> Yes, I also noticed this and do clean things, there a lot of them...
> Also the patch introduced destroy_XXX APIs but never used them, fixing 
> that too.'

Cool

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

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

Anyway, not critical now.


> 
>> Then we could pass properties objects safely to code that should only
>> have one or the other...
> Mm, might be compelling. Do we have such code actually?


Dunno

>>
>>
>> geir
>>

Mime
View raw message