This issue tracker has been migrated to GitHub ,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2014年02月11日 13:03 by Jeffrey.Armstrong, last changed 2022年04月11日 14:57 by admin. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| wcstok.default.old.patch | Jeffrey.Armstrong, 2014年02月11日 13:03 | Changes to support alternate wcstok() syntax on Windows builds | ||
| wcstok.default.try2.patch | Jeffrey.Armstrong, 2014年02月11日 17:27 | Changes to support alternate wcstok() syntax on Windows builds | review | |
| wcstok.try3.patch | Jeffrey.Armstrong, 2014年02月12日 01:03 | More complete wcstok patch | review | |
| wcstok.default.patch | Jeffrey.Armstrong, 2014年02月13日 02:33 | A simpler, complete wcstok patch | review | |
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 10286 | closed | erikjanss, 2018年11月01日 15:43 | |
| PR 11742 | erikjanss, 2019年05月19日 09:11 | ||
| Messages (17) | |||
|---|---|---|---|
| msg210935 - (view) | Author: Jeffrey Armstrong (Jeffrey.Armstrong) * | Date: 2014年02月11日 13:03 | |
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. |
|||
| msg210963 - (view) | Author: STINNER Victor (vstinner) * (Python committer) | Date: 2014年02月11日 16:29 | |
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). |
|||
| msg210980 - (view) | Author: Jeffrey Armstrong (Jeffrey.Armstrong) * | Date: 2014年02月11日 17:27 | |
I've replaced the patch with a newer version that defines Py_WCSTOK in Include/pyport.h as Victor suggested. |
|||
| msg211019 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2014年02月11日 20:53 | |
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 */ |
|||
| msg211031 - (view) | Author: STINNER Victor (vstinner) * (Python committer) | Date: 2014年02月11日 22:35 | |
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. |
|||
| msg211052 - (view) | Author: Jeffrey Armstrong (Jeffrey.Armstrong) * | Date: 2014年02月12日 01:03 | |
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. |
|||
| msg211062 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2014年02月12日 08:21 | |
LGTM. But perhaps it would be better to increase indent to 4 spaces in consistency with other indented code in this file. |
|||
| msg211069 - (view) | Author: STINNER Victor (vstinner) * (Python committer) | Date: 2014年02月12日 09:46 | |
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? |
|||
| msg211086 - (view) | Author: Jeffrey Armstrong (Jeffrey.Armstrong) * | Date: 2014年02月12日 12:30 | |
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. |
|||
| msg211123 - (view) | Author: Jeffrey Armstrong (Jeffrey.Armstrong) * | Date: 2014年02月13日 02:33 | |
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. |
|||
| msg212967 - (view) | Author: Jeffrey Armstrong (Jeffrey.Armstrong) * | Date: 2014年03月09日 17:53 | |
I didn't receive any feedback on the last patch from 2014年02月12日. Is this issue effectively dead? |
|||
| msg229174 - (view) | Author: R. David Murray (r.david.murray) * (Python committer) | Date: 2014年10月12日 16:24 | |
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 :) |
|||
| msg323101 - (view) | Author: Erik Janssens (erikjanss) * | Date: 2018年08月04日 11:36 | |
Would anyone mind if I create a new issue to reflect the current situation and create a PR to handle it with a Py_WCSTOK ? |
|||
| msg329076 - (view) | Author: Erik Janssens (erikjanss) * | Date: 2018年11月01日 15:49 | |
I created a github pull request based on the last patch of Jeffrey Armstrong. Since mingw now also has a wcstok_s function, the discrimination between wcstok/wcstok_s is now based on MS_WINDOWS being defined, as was the case in Modules/main.c. Is this correct, or should it be based on the detected compiler ? |
|||
| msg329512 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2018年11月09日 13:02 | |
wcstok_s is an optional part of C11 and can be available in other compilers. http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf#%5B%7B%22num%22%3A1401%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C0%2C792%2C0%5D |
|||
| msg329547 - (view) | Author: Erik Janssens (erikjanss) * | Date: 2018年11月09日 18:59 | |
@serhiy.storchaka Are you suggesting the discrimination should be based on compiler specific defines, or rather that it should go through configuration (a HAVE_WCSTOK_S pyconfig.h) ? |
|||
| msg342839 - (view) | Author: Erik Janssens (erikjanss) * | Date: 2019年05月19日 09:11 | |
The same issue was handled in bpo-35890 |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022年04月11日 14:57:58 | admin | set | github: 64795 |
| 2019年05月19日 09:11:19 | erikjanss | set | messages:
+ msg342839 pull_requests: + pull_request13329 |
| 2018年11月09日 19:50:05 | Jeffrey.Armstrong | set | nosy:
- Jeffrey.Armstrong |
| 2018年11月09日 18:59:12 | erikjanss | set | messages: + msg329547 |
| 2018年11月09日 13:02:48 | serhiy.storchaka | set | messages:
+ msg329512 versions: + Python 3.8, - Python 3.5 |
| 2018年11月01日 15:49:22 | erikjanss | set | messages: + msg329076 |
| 2018年11月01日 15:43:53 | erikjanss | set | pull_requests: + pull_request9597 |
| 2018年08月04日 11:36:11 | erikjanss | set | nosy:
+ erikjanss messages: + msg323101 |
| 2015年05月17日 15:21:30 | Jeffrey.Armstrong | set | status: open -> closed resolution: wont fix |
| 2014年10月12日 16:24:56 | r.david.murray | set | versions:
+ Python 3.5, - Python 3.4 nosy: + r.david.murray messages: + msg229174 stage: commit review -> patch review |
| 2014年03月09日 17:53:03 | Jeffrey.Armstrong | set | messages: + msg212967 |
| 2014年02月13日 02:33:30 | Jeffrey.Armstrong | set | files:
+ wcstok.default.patch messages: + msg211123 |
| 2014年02月12日 12:30:32 | Jeffrey.Armstrong | set | messages: + msg211086 |
| 2014年02月12日 09:46:04 | vstinner | set | messages: + msg211069 |
| 2014年02月12日 08:21:00 | serhiy.storchaka | set | messages:
+ msg211062 stage: commit review |
| 2014年02月12日 01:03:33 | Jeffrey.Armstrong | set | files:
+ wcstok.try3.patch messages: + msg211052 |
| 2014年02月11日 22:35:58 | vstinner | set | messages: + msg211031 |
| 2014年02月11日 20:53:24 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages: + msg211019 |
| 2014年02月11日 17:27:15 | Jeffrey.Armstrong | set | files:
+ wcstok.default.try2.patch messages: + msg210980 |
| 2014年02月11日 16:29:30 | vstinner | set | nosy:
+ vstinner messages: + msg210963 |
| 2014年02月11日 13:03:36 | Jeffrey.Armstrong | create | |