Hi Joe, I'm not happy with this, but I'll remove the change to asin from my patch. Thanks for looking at it.
Best regards, Goetz. > -----Original Message----- > From: Joseph D. Darcy [mailto:[email protected]] > Sent: Freitag, 9. Dezember 2016 23:29 > To: Lindenmaier, Goetz <[email protected]> > Cc: Java Core Libs <[email protected]> > Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty > coding. > > Hi Goetz, > > Please leave fdlibm asin alone as part of this change. The fdlibm > library has some anachronistic coding idioms and certainly at least in > this one case misleading code, but I'd prefer to leave the code as-is > until it is ported to Java. > > Thanks, > > -Joe > > On 12/8/2016 6:32 AM, Lindenmaier, Goetz wrote: > > Hi Joe, > > > > could you please have a look at my change to e_asin.c: > > http://cr.openjdk.java.net/~goetz/wr16/8170663- > corlib_s11y/webrev.04/src/java.base/share/native/libfdlibm/e_asin.c.udiff.html > > > > The change conserves the situation, I just added {} and comments. > > I would appreciate to clean this up as it's hard to understand and our > > code scan does not like it the way it is. > > > > We had talked about this in my thread where I learned to read this code :) > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016- > December/045165.html > > > > Best regards, > > Goetz. > > > > > >> -----Original Message----- > >> From: Martin Buchholz [mailto:[email protected]] > >> Sent: Mittwoch, 7. Dezember 2016 18:09 > >> To: Lindenmaier, Goetz <[email protected]>; Joe Darcy > >> <[email protected]> > >> Cc: Dmitry Samersoff <[email protected]>; Java Core Libs > <core- > >> [email protected]>; serviceability-dev (serviceability- > >> [email protected]) <[email protected]> > >> Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty > >> coding. > >> > >> This changes fdlibm, which has historically been untouched. Don't commit > >> without Joe Darcy's approval. > >> > >> On Wed, Dec 7, 2016 at 7:26 AM, Lindenmaier, Goetz > >> <[email protected] <mailto:[email protected]> > > wrote: > >> > >> > >> Hi Dmitry, > >> > >> yes, new_jvmpath is consistent with the other variables. > >> I also merged the ifs in SDE.c. > >> > >> new webrev: > >> http://cr.openjdk.java.net/~goetz/wr16/8170663- > >> corlib_s11y/webrev.03/ <http://cr.openjdk.java.net/~goetz/wr16/8170663- > >> corlib_s11y/webrev.03/> > >> > >> Best regards, > >> Goetz. > >> > >> > -----Original Message----- > >> > From: Dmitry Samersoff [mailto:[email protected] > >> <mailto:[email protected]> ] > >> > Sent: Wednesday, December 07, 2016 2:43 PM > >> > To: Lindenmaier, Goetz <[email protected] > >> <mailto:[email protected]> >; Java Core Libs > >> > <[email protected] <mailto:core-libs- > >> [email protected]> >; serviceability-dev (serviceability- > >> > [email protected] <mailto:[email protected]> ) > >> <[email protected] <mailto:serviceability- > >> [email protected]> > > >> > Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and > >> servicabilty > >> > coding. > >> > > >> > >> > Goetz, > >> > > >> > SDE.c: > >> > > >> > You might combine if at ll. 260 and 263 to one but it's just matter of > >> test. > >> > > >> > if (sti == baseStratumIndex || sti < 0) { > >> > return; /* Java stratum - return unchanged */ > >> > } > >> > > >> > > I'm not sure what you mean. I tried to fix it, but please > >> > > double-check the new webrev. > >> > > >> > if cnt is <= 0 loop at l.267 > >> > > >> > for (; cnt-- > 0; ++fromEntry) { > >> > > >> > is never run and we effectively do > >> > > >> > *entryCountPtr = 0; > >> > > >> > at l.283 > >> > > >> > So if we you suspect that cnt may become negative or 0: > >> > (your v.01 changes) > >> > > >> > 260 if (sti < 0 && cnt > 0) { > >> > 261 return; > >> > 262 } > >> > > >> > it's better to check it early. > >> > > >> > But I'm not sure we have to care about negative/zero cnt here. > >> > > >> > -Dmitry > >> > > >> > On 2016-12-07 11:37, Lindenmaier, Goetz wrote: > >> > > Hi Dmitry, > >> > > > >> > > thanks for looking at my change! > >> > > Updated webrev: > >> > > http://cr.openjdk.java.net/~goetz/wr16/8170663- > >> corlib_s11y/webrev.02 <http://cr.openjdk.java.net/~goetz/wr16/8170663- > >> corlib_s11y/webrev.02> > >> > > > >> > >> * src/java.base/unix/native/libjli/java_md_solinux.c > >> > >> Is this line correct? > >> > >> 519 jvmpath = JLI_StringDup(jvmpath); > >> > > > >> > > It seems pointless. Should I remove it? (The whole file is a > >> horror.) > >> > > > >> > >> * src/jdk.jdwp.agent/share/native/libjdwp/SDE.c > >> > >> It might be better to return immediately if cnt < 0, > >> > >> regardless of value of sti. > >> > > > >> > > I'm not sure what you mean. I tried to fix it, but please > >> > > double-check the new webrev. > >> > > > >> > >> * src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c > >> > >> It might be better to write > >> > >> arg.l_linger = (on) ? (unsigned short)value.i : 0; > >> > >> and leave one copy of setsockopt() call > >> > > > >> > > Yes, looks better. > >> > > > >> > > Best regards, > >> > > Goetz > >> > > > >> > > > >> > >> > >> > >> -Dmitry > >> > >> > >> > >> > >> > >> On 2016-12-06 16:12, Lindenmaier, Goetz wrote: > >> > >>> Hi, > >> > >>> > >> > >>> > >> > >>> > >> > >>> This change fixes some minor issues found in our code scans. > >> > >>> > >> > >>> I hope this correctly addresses corelib and serviceability issues. > >> > >>> > >> > >>> > >> > >>> > >> > >>> Please review: > >> > >>> > >> > >>> http://cr.openjdk.java.net/~goetz/wr16/8170663- > >> <http://cr.openjdk.java.net/~goetz/wr16/8170663-> > >> > corlib_s11y/webrev.01/ > >> > >>> > >> > >>> > >> > >>> > >> > >>> Best regards, > >> > >>> > >> > >>> Goetz. > >> > >>> > >> > >>> > >> > >>> > >> > >>> Changes in detail: > >> > >>> > >> > >>> > >> > >>> > >> > >>> e_asin.c > >> > >>> > >> > >>> Code scan reports missing {}. > >> > >>> > >> > >>> > >> > >>> The code "if (huge+x>one) {" is only there to set the inexact flag > >> of > >> > >>> the processor. > >> > >>> It's a way to avoid the C compiler to optimize the code away. It > >> is > >> > >>> always true, > >> > >>> so the parenthesis of the outer else don't matter. > >> > >>> > >> > >>> Although this basically just adds the {} I would like to submit > >> this > >> to > >> > >>> > >> > >>> avoid anybody else spends more the 30sec on understanding > >> these > >> > >>> > >> > >>> if statements. > >> > >>> > >> > >>> > >> > >>> k_standard.c > >> > >>> > >> > >>> exc.retval is returned below and thus should always be > >> initialized. > >> > >>> > >> > >>> > >> > >>> imageDecompressor.cpp > >> > >>> > >> > >>> Wrong destructor is used. Reported also by David CARLIER > >> > >>> > >> > >>> > >> > >>> java.c > >> > >>> > >> > >>> in line 1865 'name' was used, it should be 'alias'. > >> > >>> > >> > >>> > >> > >>> java_md_solinux.c > >> > >>> > >> > >>> "//" in path is useless. Further down a free is missing. > >> > >>> > >> > >>> > >> > >>> SDE.c > >> > >>> > >> > >>> Call to stratumTableIndex can return negative value if > >> defaultStratumId > >> > >>> == null. > >> > >>> > >> > >>> > >> > >>> socket_md.c > >> > >>> > >> > >>> arg.l_linger is passed to setsockopt uninitialized. Its use is > >> hidden > >> in > >> > >>> the macros. > >> > >>> > >> > >>> > >> > >>> unpack.cpp > >> > >>> > >> > >>> n.slice should not get negative argument for end, which is passed > >> from > >> > >>> dollar1. > >> > >>> But dollar1 can get negative where it is set to the result of > >> > >>> lastIndexOf(DOLLAR_MIN, DOLLAR_MAX, n, dollar2 - 1). > >> > >>> > >> > >> > >> > >> > >> > >> -- > >> > >> Dmitry Samersoff > >> > >> Oracle Java development team, Saint Petersburg, Russia > >> > >> * I would love to change the world, but they won't give me the > >> sources. > >> > > >> > > >> > -- > >> > Dmitry Samersoff > >> > Oracle Java development team, Saint Petersburg, Russia > >> > * I would love to change the world, but they won't give me the > >> sources. > >> > >>
