2006/11/30, Alex Astapchuk <[EMAIL PROTECTED]>:
Geir Magnusson Jr. wrote:
> 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
The 'const' and free() are orthogonal.
The 'const' keywords here only means kind of hint for compiler, like
'user is not allowed to change the object'. This prevents things like
temp[i] = xx at compile time.
Alex,
Thanks for noticing this.
Actually returned value should not be declared const, just because it
is a copy and user is allowed to do anything with it. Otherwise this
gives the wrong hint (both to human and compiler).
I suppose Geir basically meant just this, and this is exactly my
motivation to fix it.
just my $.02.
--
Thanks,
Alex
>
> 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.
>
>
> 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...
>
> Then we could pass properties objects safely to code that should only
> have one or the other...
>
>
> geir
>
