[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]
