8034856/8034857: More gcc warnings

[Mikael Vidstedt]

I'm inclined to agree with this. Since the code depends on a specific behavior of isspace which does not match what the system provided function does I too think it would be more robust to implement our own version of it.

Cheers,
Mikael

> On Feb 16, 2014, at 14:20, Martin Buchholz <martinrb at google.com> wrote:
> 
> Those locale-dependent APIs - more trouble than they're worth.  More often than not, you want a locale-independent version.
> 
> So just define your own is_ASCII_space etc. like everybody else has done and move on.
> 
> 
>> On Sun, Feb 16, 2014 at 9:20 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>>> On 13/02/2014 21:14, Mikael Vidstedt wrote:
>>> :
>>> 
>>> The change in question appears to come from https://bugs.openjdk.java.net/browse/JDK-6679866, but I'm not sure the bug gives enough additional information. My speculation (and it's really just a speculation) is that it's not related to isspace per-se, but to something else which gets defined/redefined/undefined by including ctype.h. I guess it would be good to know if we have tests which cover the thing the comment is alluding to (non-ascii in Premain-Class).
>> Thanks for pointing this out. I looked at it again and the issue is that isspace is a macro and depends on the locale. By not including ctype.h then it means we get linked to the libc function instead. One approach is to include ctype.h and then #undef isspace, another is to define function prototype ourselves. I think the latter is a little bit better because it would avoid accidental usage of other local sensitive char classifiers. Attached is the patch that I propose. I have deliberate moved to to after other includes so we get a chance to #undef in the event that it gets included by something else.
>> 
>> On tests then PremainClassTest.java is good enough to find this on Solaris.
>> 
>> -Alan
>> 
>> 
>> diff --git a/src/share/instrument/JarFacade.c b/src/share/instrument/JarFacade.c
>> --- a/src/share/instrument/JarFacade.c
>> +++ b/src/share/instrument/JarFacade.c
>> @@ -23,17 +23,20 @@
>>   * questions.
>>   */
>> 
>> -#ifdef _WIN32
>> -/*
>> - * Win* needs this include. However, Linux and Solaris do not.
>> - * Having this include on Solaris SPARC breaks having non US-ASCII
>> - * characters in the value of the Premain-Class attribute.
>> - */
>> -#include <ctype.h>
>> -#endif /* _WIN32 */
>>  #include <string.h>
>>  #include <stdlib.h>
>> -#include <ctype.h>
>> +
>> +/**
>> + * ctype.h is required on Windows. For other platforms we use a function
>> + * prototype to ensure that we use the libc isspace function rather than
>> + * the isspace macro (due to isspace being locale sensitive)
>> + */
>> +#ifdef _WIN32
>> +  #include <ctype.h>
>> +#else
>> +  #undef isspace
>> +  extern int isspace(int c);
>> +#endif /* _WIN32 */
>> 
>>  #include "jni.h"
>>  #include "manifest_info.h"
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140216/8c63f17b/attachment.html