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: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.
>
>
