Hi Christoph,

Some of your changes could use explanations. Can you please elaborate on the problems you are fixing in the CR? Test cases, or at least explaining how to reproduce the issues you are fixing would also be useful.

shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

VirtualMachineImpl.c: When/why is pos ever null?

error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

error_messages.h: ok. looks like just whitespace cleanup

eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

invoker.c: ok: looks like just whitespace cleanup

log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it?

I think you will need to file separate CRs for each issue. You can lump the whitespace cleanup into one. The rest probably should each be seaprate CRs, but I could see possibly doing just one CR for them if the CR documented each problem being fixed.

thanks,

Chris


On 12/4/17 7:44 AM, Langer, Christoph wrote:

Hi,

 

I’d like to contribute a few fixes that have been done in our JDK builds that add a few additional checks and cleanups. Can I please get a review.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.0/

 

Thanks,

Christoph

 


Reply via email to