Hi David, Aleksey, Thanks for the reviews! Comments inline.
On Wed, 2018-05-16 at 12:01 +1000, David Holmes wrote: > +1 on both comments. > > In addition I'd prefer > > + u4 useed = (u4)seed; > > for clarity, rather than just 's'. Fixed. > Thanks, > David > > On 16/05/2018 2:16 AM, Aleksey Shipilev wrote: > > On 05/15/2018 06:11 PM, Severin Gehwolf wrote: > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8203223 > > > webrev: > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.01/ > > > > *) Um: > > assert(seed > 0 && "invariant"); > > > > This should be this, right? > > > > assert(seed > 0, "invariant"); This was throwing me off too, but I've used the same model than other assert() calls in this file. For example line 161 in imageFile.cpp reads like this: 161 assert(replaced != NULL && "allocation failed"); If I changed it to your suggestion I'm getting this compile failure: $ bash imageFile.o.cmdline /disk/openjdk/upstream-sources/openjdk-hs/src/java.base/share/native/libjimage/imageFile.cpp:60:33: error: macro "assert" passed 2 arguments, but takes just 1 assert(seed > 0, "invariant"); ^ Seems to be a difference between hotspot and core-libs. So I've kept this as is. > > *) I would also write: > > > > return (s4)s & 0x7FFFFFFF; > > > > as > > > > return (s4)(s & 0x7FFFFFFF); Fixed. New webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.02/ Thanks, Severin
