review (S) for 6826261: class file dumping from SA is broken

[John Coomes]

Tom Rodriguez (Thomas.Rodriguez at Sun.COM) wrote:
> That's a good idea.  How about this relative to the old one:
> 
> http://cr.openjdk.java.net/~never/6826261/edit/agent/src/share/classes/sun/jvm/hotspot/tools/jcore/ClassWriter.java.udiff.html

Looks good; better than my suggestion.  One typo here:

+         // Check is flags have the attribute and if the ...
                  ^^^^

-John

> On Apr 3, 2009, at 2:03 PM, John Coomes wrote:
> 
> > Tom Rodriguez (Thomas.Rodriguez at Sun.COM) wrote:
> >> http://cr.openjdk.java.net/~never/6826261
> >
> > I'm a noob to the SA, but looks good to me.  One minor suggestion in
> > ClassWriter.java.  The common pattern for checking for
> > synthetic methods is
> >
> > 364             boolean isSyn = isSynthetic(accessFlags);
> > 365             if (isSyn && _syntheticIndex != 0)
> > 366                 fieldAttributeCount++;
> >
> > where you've added && _syntheticIndex != 0.  Can you fold this check
> > into isSyn?  E.g.:
> >
> >  boolean isSyn = _syntheticIndex != 0 && isSynthetic(accessFlags);
> >
> > The uses of isSyn (without the check for _syntheticIndex != 0) simply
> > guard a call to writeSynthetic(), and that does its own check for
> > _syntheticIndex != 0.  Except for this use in writeMethod():
> >
> > 452         if (isSyn) {
> > 453             if (DEBUG) debugMessage("\tmethod is synthetic");
> > 454             writeSynthetic();
> > 455         }
> >
> > Not sure if that debug message is still valid when _syntheticIndex ==
> > 0, since writeSynthetic() won't write anything.
> >
> > -John
> >
>