You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by "Geir Magnusson Jr." <ge...@pobox.com> on 2006/11/30 03:20:44 UTC
[drlvm] harmony-1625 - hang on a sec...
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
Re: [drlvm] harmony-1625 - hang on a sec...
Posted by Alexey Varlamov <al...@gmail.com>.
2006/11/30, Alex Astapchuk <al...@gmail.com>:
> 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
> >
>
>
>
>
Re: [drlvm] harmony-1625 - hang on a sec...
Posted by Alex Astapchuk <al...@gmail.com>.
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.
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
>
Re: [drlvm] harmony-1625 - hang on a sec...
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
Alexey Varlamov wrote:
> [snip]
>> >>
>> >> 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".
>>
>> Ok - but there should be no need to have API calls for "keys" and
>> "values", IMO.
> I'm confused now. Do you mean API naming only? Or suggest to have a
> single destroy() for all?
> For the latter, it should be possible to alloc single memory block for
> storing keys array + key strings.
I guess I simply mean that as a user of this APi, I don't want to
remember if it's a key or a value...
>>
>> >
>> >>
>> >>
>> >> 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.
>>
>> eh - they don't - there's that table number value...
> Huh? Didn't you suggest to get rid of the table number in interanl API?
Ok - not sure - I need to figure out then what is internal and what isn't...
>
>>
>> Anyway, not critical now.
> Yes. Ok, I'll play with this and see which is nicer.
>
> [snip]
Re: [drlvm] harmony-1625 - hang on a sec...
Posted by Alexey Varlamov <al...@gmail.com>.
[snip]
> >>
> >> 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".
>
> Ok - but there should be no need to have API calls for "keys" and
> "values", IMO.
I'm confused now. Do you mean API naming only? Or suggest to have a
single destroy() for all?
For the latter, it should be possible to alloc single memory block for
storing keys array + key strings.
>
> >
> >>
> >>
> >> 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.
>
> eh - they don't - there's that table number value...
Huh? Didn't you suggest to get rid of the table number in interanl API?
>
> Anyway, not critical now.
Yes. Ok, I'll play with this and see which is nicer.
[snip]
Re: [drlvm] harmony-1625 - hang on a sec...
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
Alexey Varlamov wrote:
> 2006/11/30, Geir Magnusson Jr. <ge...@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.'
Cool
>
>>
>> 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".
Ok - but there should be no need to have API calls for "keys" and
"values", IMO.
>
>>
>>
>> 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.
eh - they don't - there's that table number value...
Anyway, not critical now.
>
>> 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?
Dunno
>>
>>
>> geir
>>
Re: [drlvm] harmony-1625 - hang on a sec...
Posted by Alexey Varlamov <al...@gmail.com>.
2006/11/30, Geir Magnusson Jr. <ge...@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
>