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'.
> 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
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);