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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Krishna Addepalli

Hi All,

 

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

Webrev: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/

 

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

 

Thanks,

Krishna

Reply | Threaded
Open this post in threaded view
|

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

Sergey Bylokhov
Hi, Krishna.
One initial question about awt_Window.cpp:

2150     w = static_cast<int>((rect.right - rect.left) * scaleX /
prevScaleX);
2151     h = static_cast<int>((rect.bottom - rect.top) * scaleY /
prevScaleY);

Are you sure that we need to use "cast" instead of ScaleUp/Down?

On 29/09/2018 20:18, Krishna Addepalli wrote:

> Hi All,
>
> Please review a fix for JDK-8074824:
> https://bugs.openjdk.java.net/browse/JDK-8074824
>
> Webrev: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/ 
> <http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/>
>
> Most of the warnings have been fixed for Linux, Mac and Windows.
>
> Thanks,
>
> Krishna
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

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

Krishna Addepalli
Hi Sergey,

By using static_cast, I have maintained the existing behaviour. I’m not sure about which function to use, but should we address it separately, or here itself?

Thanks,
Krishna

> On 01-Oct-2018, at 8:33 AM, Sergey Bylokhov <[hidden email]> wrote:
>
> Hi, Krishna.
> One initial question about awt_Window.cpp:
>
> 2150     w = static_cast<int>((rect.right - rect.left) * scaleX / prevScaleX);
> 2151     h = static_cast<int>((rect.bottom - rect.top) * scaleY / prevScaleY);
>
> Are you sure that we need to use "cast" instead of ScaleUp/Down?
>
> On 29/09/2018 20:18, Krishna Addepalli wrote:
>> Hi All,
>> Please review a fix for JDK-8074824: https://bugs.openjdk.java.net/browse/JDK-8074824
>> Webrev: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/ <http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/>
>> Most of the warnings have been fixed for Linux, Mac and Windows.
>> Thanks,
>> Krishna
>
>
> --
> Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

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

Phil Race
In reply to this post by Krishna Addepalli
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)
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: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/

 

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

 

Thanks,

Krishna

Reply | Threaded
Open this post in threaded view
|

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

Phil Race
You really do need to explain *each* of the changes better.
This one .. why not NULL ?
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)
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: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/

 

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

 

Thanks,

Krishna

Reply | Threaded
Open this post in threaded view
|

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

Phil Race
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 ?
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)
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: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/

 

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

 

Thanks,

Krishna

Reply | Threaded
Open this post in threaded view
|

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

Krishna Addepalli
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 <[hidden email]> 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 ?
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) 
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
 
Most of the warnings have been fixed for Linux, Mac and Windows.
 
Thanks,
Krishna

Reply | Threaded
Open this post in threaded view
|

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

Sergey Bylokhov
In reply to this post by Krishna Addepalli
On 30/09/2018 20:48, Krishna Addepalli wrote:
> By using static_cast, I have maintained the existing behaviour. I’m not sure about which function to use, but should we address it separately, or here itself?

But the existed behavior might maintained without this fix, right? with
one exception that we will need to find all this questionable places
manually, an currently they are highlighted by the compiler.


>
> Thanks,
> Krishna
>
>> On 01-Oct-2018, at 8:33 AM, Sergey Bylokhov <[hidden email]> wrote:
>>
>> Hi, Krishna.
>> One initial question about awt_Window.cpp:
>>
>> 2150     w = static_cast<int>((rect.right - rect.left) * scaleX / prevScaleX);
>> 2151     h = static_cast<int>((rect.bottom - rect.top) * scaleY / prevScaleY);
>>
>> Are you sure that we need to use "cast" instead of ScaleUp/Down?
>>
>> On 29/09/2018 20:18, Krishna Addepalli wrote:
>>> Hi All,
>>> Please review a fix for JDK-8074824: https://bugs.openjdk.java.net/browse/JDK-8074824
>>> Webrev: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/ <http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/>
>>> Most of the warnings have been fixed for Linux, Mac and Windows.
>>> Thanks,
>>> Krishna
>>
>>
>> --
>> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

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

Krishna Addepalli
In reply to this post by Krishna Addepalli

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 <[hidden email]>
Cc: [hidden email]; 2d-dev <[hidden email]>; build-dev <[hidden email]>
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 <[hidden email]> 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 ?
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) 
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

 

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

 

Thanks,

Krishna

 


WarningInfo.txt (32K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Magnus Ihse Bursie
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 <[hidden email]>:

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 <[hidden email]>
Cc: [hidden email]; 2d-dev <[hidden email]>; build-dev <[hidden email]>
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 <[hidden email]> 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 ?
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) 
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

 

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

 

Thanks,

Krishna

 

<WarningInfo.txt>
Reply | Threaded
Open this post in threaded view
|

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

Krishna Addepalli

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 <[hidden email]>
Cc: Philip Race <[hidden email]>; [hidden email]; 2d-dev <[hidden email]>; build-dev <[hidden email]>
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 <[hidden email]>:

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 <[hidden email]>
Cc: [hidden email]; 2d-dev <[hidden email]>; build-dev <[hidden email]>
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 <[hidden email]> 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 ?
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) 
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

 

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

 

Thanks,

Krishna

 

<WarningInfo.txt>

Reply | Threaded
Open this post in threaded view
|

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

Magnus Ihse Bursie

27 nov. 2018 kl. 14:32 skrev Krishna Addepalli <[hidden email]>:

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.


If you do change it, does it trigger compilation warnings/errors elsewhere? Otherwise, I'd still recommend it, but extend the review to core-libs-dev. 

Otherwise, it might be worth opening a separate issue on JBS to properly fix SAFE_ALLOC and friends. 

/Magnus

 

Krishna

 

From: Magnus Ihse Bursie
Sent: Tuesday, November 27, 2018 6:52 PM
To: Krishna Addepalli <[hidden email]>
Cc: Philip Race <[hidden email]>; [hidden email]; 2d-dev <[hidden email]>; build-dev <[hidden email]>
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 <[hidden email]>:

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 <[hidden email]>
Cc: [hidden email]; 2d-dev <[hidden email]>; build-dev <[hidden email]>
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 <[hidden email]> 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 ?
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) 
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

 

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

 

Thanks,

Krishna

 

<WarningInfo.txt>

Reply | Threaded
Open this post in threaded view
|

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

Phil Race
In reply to this post by Krishna Addepalli
I agree. Is casting to int actually the right thing ? Definitely not always
Looking here (for example)
1042     int len;
1060     len = (int)(strlen(vendor) + 1 + strlen(renderer) + 1 + 1+strlen(version)+1 + 1);

we use len ONLY as an argument to malloc

1061     pAdapterId = malloc(len);]

So I think the correct fix is to change the declared type of len.

Others may be similar. Please review all the proposed changes with that in mind.


This has nothing to do with changing macros ..
but if we do that an option is to use desktop specific macros .. we have 
already have SAFE_TO_ALLOC_2 and SAFE_TO_ALLOC_3 desktop module macros.

Also I'd really like to see what the compiler warnings are that you are fixing.
Else I find these kinds of changes difficult to review.
Please enumerate the warnings along with explanationo why the fix is "right"

And since you are changing shared code you need to test this on ALL platforms,
even if you are fixing just gcc warnings.


-phil.
On 11/27/18 5:32 AM, Krishna Addepalli wrote:

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 [hidden email]
Cc: Philip Race [hidden email]; [hidden email]; 2d-dev [hidden email]; build-dev [hidden email]
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 <[hidden email]>:

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 <[hidden email]>
Cc: [hidden email]; 2d-dev <[hidden email]>; build-dev <[hidden email]>
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 <[hidden email]> 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 ?
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) 
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

 

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

 

Thanks,

Krishna

 

<WarningInfo.txt>