Support for alternate wcstok syntax for Windows compilers

[JeffreyArmstrong]

BPO	20596
Nosy	@vstinner, @bitdancer, @serhiy-storchaka, @erikjanss
PRs	bpo-20596: define a platform dependent Py_WCSTOK in Include/pyport.h,… #10286 bpo-35890 : Fix some API calling consistency #11742
Files	wcstok.default.old.patch: Changes to support alternate wcstok() syntax on Windows builds wcstok.default.try2.patch: Changes to support alternate wcstok() syntax on Windows builds wcstok.try3.patch: More complete wcstok patch wcstok.default.patch: A simpler, complete wcstok patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-05-17.15:21:30.564>
created_at = <Date 2014-02-11.13:03:36.344>
labels = ['interpreter-core', 'build', '3.8', 'OS-windows']
title = 'Support for alternate wcstok syntax for Windows compilers'
updated_at = <Date 2019-05-19.09:11:19.855>
user = 'https://bugs.python.org/JeffreyArmstrong'

bugs.python.org fields:

activity = <Date 2019-05-19.09:11:19.855>
actor = 'erikjanss'
assignee = 'none'
closed = True
closed_date = <Date 2015-05-17.15:21:30.564>
closer = 'Jeffrey.Armstrong'
components = ['Interpreter Core', 'Windows']
creation = <Date 2014-02-11.13:03:36.344>
creator = 'Jeffrey.Armstrong'
dependencies = []
files = ['34038', '34045', '34055', '34063']
hgrepos = []
issue_num = 20596
keywords = ['patch']
message_count = 17.0
messages = ['210935', '210963', '210980', '211019', '211031', '211052', '211062', '211069', '211086', '211123', '212967', '229174', '323101', '329076', '329512', '329547', '342839']
nosy_count = 4.0
nosy_names = ['vstinner', 'r.david.murray', 'serhiy.storchaka', 'erikjanss']
pr_nums = ['10286', '11742']
priority = 'normal'
resolution = 'wont fix'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue20596'
versions = ['Python 3.8']

[JeffreyArmstrong]

In two locations, the current interpreter code makes some assumptions concerning the syntax of the wcstok() function based solely on the operating system (Windows, in this case). Compilers other than MSVC may (and do) provide alternative wcstok() syntaxes.

The first change in the attached patch changes a preprocessor check in Modules/main.c to determine if we're compiling with MSVC rather than just whether we're compiling with Windows. If so, it uses Windows's basic two-argument wcstok() function as it always has. If the compiler isn't MSVC, the code will now default to the Unix method of converting to ASCII first before tokenizing. This change is more sensible because the code should really be checking for the compiler's wcstok() capabilities, not what operating system Python is being compiled for.

The second change in the attached patch adds some new code to PC/getpathp.c to support alternate wcstok() syntax in the find_env_config_value() function. A preprocessor check will now determine if we're compiling for MSVC and, if so, default to the three-argument wcstok_s() function. If the almost-compatible Open Watcom compiler is detected, a three-argument, POSIX-like wcstok() function is used. If another compiler is detected, the original two-argument wcstok() is assumed to be adequate.

[vstinner]

IMO your WCSTOK macro should be called Py_WCSTOK and moved to Include/pyport.h, so you can use it in Modules/main.c (please use Unicode for environment variables on Windows).

[JeffreyArmstrong]

I've replaced the patch with a newer version that defines Py_WCSTOK in Include/pyport.h as Victor suggested.

[serhiy-storchaka]

The code can be a little more clear if use indentation in preprocessor directives:

#ifdef MS_WINDOWS
# ifdef _MSC_VER
#  define Py_WCSTOK(x,y,z)  wcstok_s(x,y,z)
# else
#  ifdef __WATCOMC__
#   define Py_WCSTOK(x,y,z)  wcstok(x,y,z)
#  else
#   define Py_WCSTOK(x,y,z)  wcstok(x,y)
#  endif /* __WATCOMC__ */
# endif /* _MSC_VER */
#endif /* MS_WINDOWS */

Or may be even using #elif:

#ifdef MS_WINDOWS
# if defined(_MSC_VER)
#  define Py_WCSTOK(x,y,z)  wcstok_s(x,y,z)
# elif defined(__WATCOMC__)
#  define Py_WCSTOK(x,y,z)  wcstok(x,y,z)
# else
#  define Py_WCSTOK(x,y,z)  wcstok(x,y)
# endif
#endif /* MS_WINDOWS */

[vstinner]

wcstok() is available on other platforms like Linux. You may also
define the constant for these platforms, no?

On Linux, it has 3 parameters:
wchar_t *wcstok(wchar_t *wcs, const wchar_t *delim, wchar_t **ptr);

You should explain that the macro may ignore the third parameter in a
comment and maybe use better names for parameters.

[JeffreyArmstrong]

Based on the comments thus far, I've gone ahead with another version of this patch. Py_WCSTOK is now defined regardless of OS. For Windows, it chooses between MSVC's wcstok_s, Open Watcom's wcstok, and MinGW's/misc's two-argument wcstok. If the platform isn't Windows, it defaults to the POSIX-like three-argument wcstok (same as Open Watcom's, actually).

The wcstok functionality is really only used in three places: PC/getpathp.c, Modules/getpath.c, and Modules/main.c. This patch changes the calls to Py_WCSTOK in all cases.

I appreciate the consideration and input this patch is receiving.

[serhiy-storchaka]

LGTM.

But perhaps it would be better to increase indent to 4 spaces in consistency with other indented code in this file.

[vstinner]

I don't think that "#ifdef MS_WINDOWS" is very useful, you can drop it. So get something simpler:

/* Portable macro for wcstok(): on Windows, it is not thread-safe, the state
 * argument is ignored, except if Visual Studio is used. */
#ifdef _MSC_VER
#  define Py_WCSTOK(str, tok, state)  wcstok_s(str, tok, state)
#elif defined(MS_WINDOWS) && !defined(__WATCOMC__)
#  define Py_WCSTOK(str, tok, state)  wcstok(str, tok)
#else
#  define Py_WCSTOK(str, tok, state)  wcstok(str, tok, state)
#endif

But I don't like "#elif defined(MS_WINDOWS) && !defined(WATCOMC)": what are the other compilers on Windows which provide wcstok() with 2 parameters?

[JeffreyArmstrong]

I know that Borland/Embarcadero also uses the two-argument version like Microsoft. As I said earlier, MinGW uses the two-argument version, although it is currently marked as deprecated. Just from searching, it appears that Pelles C/LCC uses the POSIX-like three-argument version. I'm having trouble thinking of any further Windows C compilers at the moment.

The purpose of wrapping the definition in MS_WINDOWS originally was to preserve any behavior that already existed when building with a compiler that uses the two-argument version on Windows without any explicit case being called out for said compiler.

[JeffreyArmstrong]

I've included a revised patch that is far simpler than the others I've proposed. In this example, the preprocess checks for MSVC or Borland/Embarcadero and, if found, uses three-argument wcstok_s(). If neither is detected, it checks for MinGW and, if found, defaults to old two-argument wcstok(). Otherwise, it defaults to three-argument wcstok().

The default in this case effectively encompasses all other OSes where POSIX-like wcstok() is available. Additionally, it also allows this default for Open Watcom on Windows or any other theoretically possible compilers on Windows that may provide this more common wcstok() syntax.

[JeffreyArmstrong]

I didn't receive any feedback on the last patch from 2014-02-12. Is this issue effectively dead?

[bitdancer]

Moving this back to 'patch review' stage since there's a new patch that hasn't been reviewed. Perhaps this 'ping' will result in someone doing the review (so, no, the issue isn't dead, just sleeping :)