-----Original Message-----
From: Martin Buchholz [mailto:marti...@google.com]
Sent: Mittwoch, 7. Dezember 2016 18:09
To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Joe Darcy
<joe.da...@oracle.com>
Cc: Dmitry Samersoff <dmitry.samers...@oracle.com>; Java Core Libs <core-
libs-...@openjdk.java.net>; serviceability-dev (serviceability-
d...@openjdk.java.net) <serviceability-...@openjdk.java.net>
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
<goetz.lindenma...@sap.com <mailto:goetz.lindenma...@sap.com> > 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:dmitry.samers...@oracle.com
<mailto:dmitry.samers...@oracle.com> ]
> Sent: Wednesday, December 07, 2016 2:43 PM
> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com
<mailto:goetz.lindenma...@sap.com> >; Java Core Libs
> <core-libs-dev@openjdk.java.net <mailto:core-libs-
d...@openjdk.java.net> >; serviceability-dev (serviceability-
> d...@openjdk.java.net <mailto:d...@openjdk.java.net> )
<serviceability-...@openjdk.java.net <mailto:serviceability-
d...@openjdk.java.net> >
> 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.