[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR(xxxs): 8200052: libjavajpeg: Fix compile warning in jchuff.c
From: Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date: 2018-03-27 18:39:59
Message-ID: CAA-vtUycbXVrJKmdW0ZOMnvw_f-jAL_RNcuF7xtu6Jb+okBHXw () mail ! gmail ! com
[Download RAW message or body]
Hi Phil,
On Tue, Mar 27, 2018 at 6:44 PM, Phil Race <philip.race@oracle.com> wrote:
> As I said I prefer the make file change, since this is a change to an
> upstream library.
> I've looked at jpeg-9c and it still looks identical to what we have in 6b,
> so this code
> seems to have stood the test of time. I'm also unclear why the compiler
> would
> complain about that and not the one a few lines later
>
>
> 819 while (bits[i] == 0) /* find largest codelength still in use */
> 820 i--;
>
> A push to jchuff.c will get blown away if/when we upgrade.
> A tool-chain specific fix in the makefile with an appropriate comment is more targeted.
>
>
> -phil.
>
>
I dislike switching off the compiler warning, since it may be actually
useful. Also I cannot exclude the possibility (I stared at the code some
time already) that the warning is actually meaningful and that we actually
should fix this underflow. Can you?
However, I see your arguments, and since you object to this patch, I revoke
it. Unfortunately I am out of time. I think for the time being we will
continue building with --disable-warnings-as-errors on zlinux.
Thanks, Thomas
>
> On 03/27/2018 05:44 AM, Thomas Stüfe wrote:
>
> Hi all,
>
> just a friendly reminder. I would like to push this tiny fix because
> tripping over this on our linux s390x builds is annoying (yes, we can
> maintain patch queues, but this is a constant error source).
>
> I will wait for 24 more hours until a reaction. If no serious objections
> are forcoming, I want to push it (tier1 tests ran thru, and me and
> Christoph langer are both Reviewers).
>
> Thanks! Thomas
>
> On Wed, Mar 21, 2018 at 6:20 PM, Thomas Stüfe <thomas.stuefe@gmail.com>
> wrote:
>
>> Hi all,
>>
>> may I please have another review for this really trivial change. It fixes
>> a gcc warning on s390 and ppc. Also, it is probably a good idea to fix this.
>>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8200052
>> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8200052-
>> fix-warning-in-jchuff.c/webrev.00/webrev/
>>
>> This was contributed by Adam Farley at IBM.
>>
>> I already reviewed this. I also test-built on zlinux and it works.
>>
>> Thanks, Thomas
>>
>
>
>
[Attachment #3 (text/html)]
<div dir="ltr">Hi Phil,<div><br></div><div class="gmail_extra"><div \
class="gmail_quote">On Tue, Mar 27, 2018 at 6:44 PM, Phil Race <span dir="ltr"><<a \
href="mailto:philip.race@oracle.com" \
target="_blank">philip.race@oracle.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
As I said I prefer the make file change, since this is a change to
an upstream library.<br>
I've looked at jpeg-9c and it still looks identical to what we have
in 6b, so this code<br>
seems to have stood the test of time. I'm also unclear why the
compiler would<br>
complain about that and not the one a few lines later <br>
<br>
<pre> 819 while (bits[i] == 0) /* find largest codelength still in use \
*/ 820 i--;
A push to jchuff.c will get blown away if/when we upgrade.
A tool-chain specific fix in the makefile with an appropriate comment is more \
targeted. </pre><span class="HOEnZb"><font color="#888888">
<br>
-phil.</font></span><div><div \
class="h5"><br></div></div></div></blockquote><div><br></div><div>I dislike switching \
off the compiler warning, since it may be actually useful. Also I cannot exclude the \
possibility (I stared at the code some time already) that the warning is actually \
meaningful and that we actually should fix this underflow. Can \
you?</div><div><br></div><div>However, I see your arguments, and since you object to \
this patch, I revoke it. Unfortunately I am out of time. I think for the time being \
we will continue building with --disable-warnings-as-errors on \
zlinux.</div><div><br></div><div>Thanks, Thomas</div><div><br></div><div> \
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><div><div class="h5"> \
<br> <div class="m_-1038829432852292963moz-cite-prefix">On 03/27/2018 05:44 AM, \
Thomas Stüfe wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>just a friendly reminder. I would like to push this tiny
fix because tripping over this on our linux s390x builds is
annoying (yes, we can maintain patch queues, but this is a
constant error source).</div>
<div><br>
</div>
<div>I will wait for 24 more hours until a reaction. If no
serious objections are forcoming, I want to push it (tier1
tests ran thru, and me and Christoph langer are both
Reviewers).</div>
<div><br>
</div>
<div>Thanks! Thomas</div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Wed, Mar 21, 2018 at 6:20 PM, Thomas
Stüfe <span dir="ltr"><<a href="mailto:thomas.stuefe@gmail.com" \
target="_blank">thomas.stuefe@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px \
#ccc solid;padding-left:1ex"> <div dir="ltr">Hi all,
<div><br>
</div>
<div>may I please have another review for this really
trivial change. It fixes a gcc warning on s390 and ppc.
Also, it is probably a good idea to fix this.</div>
<div><br>
</div>
<div>bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8200052" \
target="_blank">https://bugs.openjdk.java<wbr>.net/browse/JDK-8200052</a></div> \
<div>webrev: <a href="http://cr.openjdk.java.net/%7Estuefe/webrevs/8200052-fix-warning-in-jchuff.c/webrev.00/webrev/" \
target="_blank">http://cr.openjdk.java<wbr>.net/~stuefe/webrevs/8200052-<wbr>fix-warning-in-jchuff.c/<wbr>webrev.00/webrev/</a></div>
<div><br>
</div>
<div>This was contributed by Adam Farley at IBM.</div>
<div><br>
</div>
<div>I already reviewed this. I also test-built on zlinux
and it works.</div>
<div><br>
</div>
<div>Thanks, Thomas</div>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic