Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: pass (herse.apache.org: domain of alexey.v.varlamov@gmail.com
 designates 66.249.82.228 as permitted sender)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=gmail.com;
        h=received:message-id:date:from:to:subject:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references;
        b=aCr31NUKZpWKHXt/aTOVVuVRpoOeBFWmws2VO/R7+FXjs1dXi44ny/3meyZYCeTbFHgnL0hED1KyGEA6Jk740JTe/E4X+g9QyBJB6g9wjUpOfe0E1aYTKaliSmDPwZ8hxR9jcGM7ulq5padQn9D0YIY6P0SM5wC17VosUxbptGw=
Message-ID: <f944d83d0611291849n4eed0151qc4fed67e5925d8cc@mail.gmail.com>
Date: Thu, 30 Nov 2006 08:49:08 +0600
From: "Alexey Varlamov" <alexey.v.varlamov@gmail.com>
To: dev@harmony.apache.org, geir@pobox.com
Subject: Re: [drlvm] harmony-1625 - hang on a sec...
In-Reply-To: <456E3FFC.3030804@pobox.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
References: <456E3FFC.3030804@pobox.com>

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
>