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 02:49:08 GMT
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.

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

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

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

Mime
View raw message