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]
