[OpenJDK 2D-Dev] <AWT Dev> [12]RFR: [JDK-8074824]: Resolve disabled warnings for libawt_xawt

Tue Nov 27 13:32:19 UTC 2018

Hi Magnus,

Thanks for taking a look. I was wanting to change the SAFE_ALLOC definition, but since that file is in java.base, I was not sure of changing it.

Krishna

From: Magnus Ihse Bursie 
Sent: Tuesday, November 27, 2018 6:52 PM
To: Krishna Addepalli <krishna.addepalli at oracle.com>
Cc: Philip Race <philip.race at oracle.com>; awt-dev at openjdk.java.net; 2d-dev <2d-dev at openjdk.java.net>; build-dev <build-dev at openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> [12]RFR: [JDK-8074824]: Resolve disabled warnings for libawt_xawt

I normally do not comment on component source code changes, but I glanced through this and noticed that a lot of size_t values are casted to int, in situations where a size_t is expected, like SAFE_ALLOC or so. Perhaps it would be better to change the argument to those functions, rather than to cast a lot of size_t expressions to int. As a rule of thumb, any expression that measure an amount of memory should be of type size_t, rather than int. 

/Magnus

27 nov. 2018 kl. 12:14 skrev Krishna Addepalli <HYPERLINK "mailto:krishna.addepalli at oracle.com"krishna.addepalli at oracle.com>:

Hi Phil,

To reduce the scope, I have created a new webrev, which addresses only warnings on Linux platform.

Warnings for other platforms will be addressed in separate bugs.

Here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev02/

For your reference, I’m attaching the warning log generated by the compiler for each warning type. Hope this helps in the review.

I ran the all the jtreg tests, but I’m not sure if the changes have caused any problems. 

I checked with Ajit (who tried to address this issue before), and ran SwingSet2 with GTK2 and GTK3 and did not find any crashes.

Thanks,

Krishna

From: Krishna Addepalli 
Sent: Tuesday, October 2, 2018 8:53 PM
To: Philip Race <HYPERLINK "mailto:philip.race at oracle.com"philip.race at oracle.com>
Cc: HYPERLINK "mailto:awt-dev at openjdk.java.net"awt-dev at openjdk.java.net; 2d-dev <HYPERLINK "mailto:2d-dev at openjdk.java.net"2d-dev at openjdk.java.net>; build-dev <HYPERLINK "mailto:build-dev at openjdk.java.net"build-dev at openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> [12]RFR: [JDK-8074824]: Resolve disabled warnings for libawt_xawt

Yes, that is right.

I have compiled it Mac, Linux and Windows locally. I tried submitting a Mach5 job, but was unable to as it was down. Will try it again.

Thanks

Krishna

On 02-Oct-2018, at 3:39 AM, Philip Race <HYPERLINK "mailto:philip.race at oracle.com"philip.race at oracle.com> wrote:

I suspect I understand this one now .. the array is stack allocated so we don't want NULL
but the compiler probably complained about possible uninitialised use of the values ?

-phil.

On 10/1/18, 9:38 AM, Philip Race wrote:

You really do need to explain *each* of the changes better.
This one .. why not NULL ?
HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/src/java.desktop/share/native/libawt/java2d/loops/ProcessPath.c.udiff.html"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/src/java.desktop/share/native/libawt/java2d/loops/ProcessPath.c.udiff.html

-phil

On 10/1/18, 9:19 AM, Philip Race wrote:

Hi,

1) Has this been built on all platforms ?
2)  I can't find the list of warnings that you are seeing and fixing and they are all over the place.
So adding 2d-dev and build-dev.
For each of these changes, please show what was the warning that you received from the compiler
This might sound like a lot of work, but it won't be disproportionate and I've made the same
request for similar reviews and without it, it is hard to review the changes.

For example (and I do mean just example) 
HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/src/java.desktop/unix/native/common/awt/awt_Font.c.udiff.html"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/src/java.desktop/unix/native/common/awt/awt_Font.c.udiff.html

why would that not be #ifdef instead ?

3) Testing .. did you run at least all our jtreg tests to make sure you didn't break
some behaviour ..

-phil.

On 9/29/18, 8:18 PM, Krishna Addepalli wrote:

Hi All, 

Please review a fix for JDK-8074824: https://bugs.openjdk.java.net/browse/JDK-8074824

Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/

Most of the warnings have been fixed for Linux, Mac and Windows.

Thanks,

Krishna

<WarningInfo.txt>