BUG: Fix test_from_object_array_unicode (test_defchararray.TestBasic)…

[tadeu]

… on Windows debug

"isspace" crashes with an assert on debug when the character is not ASCII, in Windows

[charris]

This isn't actually a TST, that prefix is for official tests in tests directories.

[charris]

Lines < 80 characters.

[tadeu]

Lines < 80 characters.

The largest line in the patch have 76 chars.
Or do you mean this line

#define isspace(c) ((c==' ')||(c=='\t')||(c=='\n')||(c=='\r')||(c=='\v')||(c=='\f'))

which was already there?

[charris]

Yep, might as well fix it.

[tadeu]

This isn't actually a TST, that prefix is for official tests in tests directories.

Sorry, I've seem some TST commits with fixes for broken tests and assumed this would be too. What would be the correct prefix here?

[charris]

That would be a simple BUG.

[tadeu]

That would be a simple BUG.

Ok, I've updated it.

[charris]

Hmm, there is a NumPyOS_ascii_isspace in numpyos.{c,h} that does the same thing. I wonder if we should be using that? There are several other uses of isspace that should perhaps be changed.

EDIT: Seems tne standard isspace is locale dependent, but we override is here to be standard C locale.

EDIT2: But isspace is also used for ucs4, which pretty much requires a macro.

[charris]

I'm wondering if we should make the macro definition unconditional? The other option is to use NumPyOS_ascii_isspace but change its signature to take an int instead of char.

[tadeu]

Ok, so what do you suggest? Something like this?

NPY_NO_EXPORT int
-NumPyOS_ascii_isspace(char c)
+NumPyOS_ascii_isspace(int c)
{
    return c == ' ' || c == '\f' || c == '\n' || c == '\r' || c == '\t'
                    || c == '\v';
}

-#define isspace(c)  ((c==' ')||(c=='\t')||(c=='\n')||(c=='\r')||(c=='\v')||(c=='\f'))
+#define isspace(c)  NumPyOS_ascii_isspace(c)

Edit: now that I saw your edit. Yes, if it must support ucs4, it better continue as a macro, right? So what should I do, only break down the long line?

[charris]

I think the change to int for the argument takes care of ucs4, I don't think we run on any platforms where int isn't at least four bytes, so type promotion should be automatic. There are only a few uses of isspace in the file, just the two routines, so I'd get rid of the define and use the function directly. The ascii spaces are required for ucs4 in any case.

[charris]

Maybe want to use a cast to int for the (unsigned) ucs4. My machine has 64 bit ints, so no problem, but might be needed for machines with 32 bit ints.

[tadeu]

Should I change the other NumPyOS_ascii_* functions too? (e.g., NumPyOS_ascii_isalpha, NumPyOS_ascii_isdigit, etc.)

[tadeu]

I've updated the commit

[charris]

Isn't numpyos.c being linked into multiarray?

What happened to this comment?

[tadeu]

Isn't numpyos.c being linked into multiarray?

What happened to this comment?

Sorry, I deleted it, it was a mistake I made and quickly corrected (was
calling the wrong function) :)

[charris]

OK, let's give this a shot. I think it should be harmless (knock, knock). I'm not sure where all the other problems with the debug version come from. There is a special Python2.7 msvc compiler, Microsoft Visual C++ Compiler for Python 2.7 , that should be compatible with Python 2.7. I don't know if that makes a difference. Note also that all python extensions need to be compiled with the same compiler. MSVC 2010 is much later than the official 2.7 compiler, vs2008.

[charris]

Thanks @tadeu .

[tadeu]

Thanks @charris !
Yes, unfortunately, I'm currently stuck using a version compiled with MSVC 2010 (and this means all other extensions too), because I'm interacting with other C++ code that depends on this compiler.
But if the other debug problems persist, I'll try to see if I can reproduce them with the vs2008 standard version.