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 <marti...@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.bate...@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" >
