2006/11/30, Geir Magnusson Jr. <[EMAIL PROTECTED]>:
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
