harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Geir Magnusson Jr." <g...@pobox.com>
Subject [drlvm] harmony-1625 - hang on a sec...
Date Thu, 30 Nov 2006 02:20:44 GMT
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

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

Mime
View raw message