[OpenJDK 2D-Dev] RFR(xxxs): 8200052: libjavajpeg: Fix compile warning in jchuff.c

[Thomas Stüfe]

Same here. I would like to have this fix in, but do not want to go
over Phils head.

I think Phil was the main objector, maybe he could reconsider?`

Thanks, Thomas

On Thu, Apr 26, 2018 at 10:39 AM, Magnus Ihse Bursie
<magnus.ihse.bursie at oracle.com> wrote:
> I don't object, but it's not build code so I don't have the final say.
>
> /Magnus
>
>
> On 2018-04-25 17:43, Adam Farley8 wrote:
>
> Hi All,
>
> Does anyone have an objection to pushing this tiny change in?
>
> It doesn't break anything, it fixes a build break on two supported
> platforms, and it seems
> like we never refresh the code from upstream.
>
> - Adam
>
>> I also advocate the source code fix, as Make isn't meant to use the sort
>> of logic required
>> to properly analyse the toolchain version string.
>>
>> e.g. An "eq" match on 4.8.5 doesn't protect the user who is using 4.8.6,
>> and Make doesn't
>> seem to do substring stuff unless you mess around with shells.
>>
>> Plus, as people have said, it's better to solve problem x (or work around
>> a specific
>> instance of x) than to ignore the exception, even if the ignoring code is
>> toolchain-
>> specific.
>>
>> - Adam Farley
>>
>> > On 2018-03-27 18:44, Phil Race wrote:
>> >
>> >> As I said I prefer the make file change, since this is a change to an
>> >> upstream library.
>> >
>> > Newtons fourth law: For every reviewer, there's an equal and opposite
>> > reviewer. :)
>> >
>> > Here I am, advocating a source code fix. As Thomas says, the compiler
>> > complaint seems reasonable, and disabling it might cause us to miss other
>> > future errors.
>> >
>> > Why can't we push the source code fix, and also submit it upstream?
>> >
>> > /Magnus
>> >
>> >
>> > 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,
>>
>> Returning to this.
>>
>> While I understand your reluctance to patch upstream code, let me point
>> out that we have not updated libjpeg a single time since the JDK was open
>> sourced. We're using 6b from 27-Mar-1998. I had a look at the Wikipedia page
>> on libjpeg, and this is the latest "uncontroversial" version of the source.
>> Versions 7 and up have proprietary extensions, which in turn has resulted in
>> multiple forks of libjpeg. As it stands, it seems unlikely that we will ever
>> replace libjpeg 6b with a simple upgrade from upstream.
>>
>> I therefore maintain my position that a source code fix would be the best
>> way forward here.
>>
>> /Magnus
>>
>> >
>> >
>> > -phil.
>> >
>> >
>> > 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 at 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
>> >
>>
>> Unless stated otherwise above:
>> IBM United Kingdom Limited - Registered in England and Wales with number
>> 741598.
>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>>
>>
>
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>
>