On 03/26/2013 07:24 PM, serguei.spit...@oracle.com wrote:
Copy usually means copying values by pointers.
Not clear what's wrong with the "set_annotations" as there is no such function yet.


There are set_x_annotations and I wouldn't want someone to think set_annotations just sets one sort. I don't know, it's just opinion.

Probably, you find it confusing too.
But I'm not insisting on the copy_annotation_pointers() either. :)
Let's keep it as it is.

Thanks for the code review!
Coleen


Thanks,
Serguei


On 3/26/13 3:58 PM, Coleen Phillimore wrote:

It copies the pointers. I can't change it to set_annotations because there are already set_ functions. O could change to copy_annotation_pointers() if you insist.

Coleen

On 03/26/2013 07:00 PM, serguei.spit...@oracle.com wrote:
Coleen,

This does not look like a clone or copy, it just sets the value?
  366 void ConstMethod::copy_annotations(ConstMethod* cm) {
  367   if (cm->has_method_annotations()) {
  368     assert(has_method_annotations(), "should be allocated already");
  369     set_method_annotations(cm->method_annotations());
  370   }
  ...

Do we have to actually clone the annotations?
If not, then the name "copy_annotations" is wrong.
It must be "set_annotations".

The test fixes look Ok.

Thanks,
Serguei


On 3/26/13 3:11 PM, Coleen Phillimore wrote:
Summary: Neglected to copy the annotations in clone_with_new_data when they were moved to ConstMethod.

open webrev at http://cr.openjdk.java.net/~coleenp/8009531/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8009531

Also. please review JDK test modified to test that this crash is fixed (will check in in two weeks).

open webrev at http://cr.openjdk.java.net/~coleenp/8009531_jdk

Thanks,
Coleen










Reply via email to