Skip to content

bpo-28269: Replace strcasecmp with system function stricmp#13095

Merged
serhiy-storchaka merged 4 commits intopython:masterfrom
gongminmin:RemoveStrcasecmp
Jul 13, 2019
Merged

bpo-28269: Replace strcasecmp with system function stricmp#13095
serhiy-storchaka merged 4 commits intopython:masterfrom
gongminmin:RemoveStrcasecmp

Conversation

@gongminmin
Copy link
Copy Markdown
Contributor

@gongminmin gongminmin commented May 5, 2019 •
edited by bedevere-bot
Loading

The idea is from a comment in the issue. Replace strcasecmp in Python/dynload_win.c to posix stricmp,

  1. MinGW can compile this file,
  2. Don't need to maintain strcasecmp,
  3. No #ifdef/#endif. The code is converged between compilers.

https://bugs.python.org/issue28269

@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented May 5, 2019

Microsoft deprecated stricmp and recommends using _stricmp instead.

@gongminmin
Copy link
Copy Markdown
Contributor Author

gongminmin commented May 5, 2019 •
edited
Loading

Just tried, MinGW also accept _stricmp. I'll use it instead. Thanks @serhiy-storchaka .

BoboTiG
BoboTiG reviewed May 28, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@BoboTiG BoboTiG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @gongminmin :)

@skrah, do you think it is worth deleting those lines too?

cpython/Modules/_decimal/libmpdec/vccompat.h

Lines 46 to 47 in 3880f26

#undef strcasecmp
#define strcasecmp _stricmp

And also update this comment to remove any occurrences of strcasecmp?

cpython/Modules/_decimal/libmpdec/io.c

Line 55 in 74d7f76

* Work around the behavior of tolower() and strcasecmp() in certain

Comment thread Misc/NEWS.d/next/Windows/2019-05-05-05-23-34.bpo-28269.-MOHI7.rst Outdated
@skrah
Copy link
Copy Markdown
Contributor

skrah commented May 28, 2019

@skrah, do you think it is worth deleting those lines too?

No, vccompat.h intentionally translates MSVC functions to Unix functions. Also, libmpdec needs to stay in sync with upstream. But the current code organization is absolutely deliberate.

@gongminmin
Copy link
Copy Markdown
Contributor Author

gongminmin commented Jul 10, 2019

Ping?

@serhiy-storchaka serhiy-storchaka merged commit 05f2d84 into python:master Jul 13, 2019
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jul 13, 2019

Thanks @gongminmin for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7, 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jul 13, 2019

GH-14740 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 13, 2019
@gongminmin @miss-islington
bpo-28269: Replace strcasecmp with system function _stricmp. (pythonG…
a52ab0f 
…H-13095)

(cherry picked from commit 05f2d84)

Co-authored-by: Minmin Gong <[email protected]>
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jul 13, 2019

GH-14741 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 13, 2019
@gongminmin @miss-islington
bpo-28269: Replace strcasecmp with system function _stricmp. (pythonG…
b4966d1 
…H-13095)

(cherry picked from commit 05f2d84)

Co-authored-by: Minmin Gong <[email protected]>
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jul 13, 2019

Sorry, @gongminmin and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 05f2d84cae4ba1ff15b7a1d0347305393f4bdcc5 2.7

@gongminmin gongminmin deleted the RemoveStrcasecmp branch July 14, 2019 01:53
miss-islington added a commit that referenced this pull request Aug 24, 2019
@miss-islington @gongminmin
bpo-28269: Replace strcasecmp with system function _stricmp. (GH-13095)
920ec4b 
(cherry picked from commit 05f2d84)

Co-authored-by: Minmin Gong <[email protected]>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@gongminmin @lisroach
bpo-28269: Replace strcasecmp with system function _stricmp. (pythonG…
c18f5b5 
…H-13095)
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@gongminmin @DinoV
bpo-28269: Replace strcasecmp with system function _stricmp. (pythonG…
8570d1a 
…H-13095)
@serhiy-storchaka serhiy-storchaka removed their assignment Dec 29, 2020
jcfr added a commit to python-cmake-buildsystem/cpython that referenced this pull request Jan 18, 2022
@jcfr @davidsansome
MinGW support: Add patches-win32/01-dynload_win.patch
c3a3ad9 
This commit integrates changes originally added to the project
as python-cmake-buildsystem/python-cmake-buildsystem@18d49757e (Mingw32 support, and add cmake
options for disabling)

Note that in  more recent version of Python, this patch is obsoleted
by the following commits:

  See python/cpython@589f89e2a (introduced in 3.3)
      Removed a Windows 9x trick used before LoadLibraryExW.
      Windows 9x has long been unsupported and the result of GetFullPathName was not even
      being used in the first place.

  See python/cpython@05f2d84ca (introduced in 3.9)
      bpo-28269: Replace strcasecmp with system function _stricmp. (pythonGH-13095)

Co-authored-by: David Sansome <[email protected]>
jcfr added a commit to python-cmake-buildsystem/cpython that referenced this pull request Jan 18, 2022
@jcfr @davidsansome
MinGW support: Add patches-win32/01-dynload_win.patch
a3b11fa 
This commit integrates changes originally added to the project
as python-cmake-buildsystem/python-cmake-buildsystem@18d49757e (Mingw32 support, and add cmake
options for disabling)

Note that in  more recent version of Python, this patch is obsoleted
by the following commits:

  See python/cpython@589f89e2a (introduced in 3.3)
      Removed a Windows 9x trick used before LoadLibraryExW.
      Windows 9x has long been unsupported and the result of GetFullPathName was not even
      being used in the first place.

  See python/cpython@05f2d84ca (introduced in 3.9)
      bpo-28269: Replace strcasecmp with system function _stricmp. (pythonGH-13095)

Co-authored-by: David Sansome <[email protected]>
jcfr added a commit to python-cmake-buildsystem/cpython that referenced this pull request Jan 18, 2022
@jcfr @davidsansome
MinGW support: Add patches-win32/01-dynload_win.patch
5674be5 
This commit integrates changes originally added to the project
as python-cmake-buildsystem/python-cmake-buildsystem@18d49757e (Mingw32 support, and add cmake
options for disabling)

Note that in  more recent version of Python, this patch is obsoleted
by the following commits:

  See python/cpython@589f89e2a (introduced in 3.3)
      Removed a Windows 9x trick used before LoadLibraryExW.
      Windows 9x has long been unsupported and the result of GetFullPathName was not even
      being used in the first place.

  See python/cpython@05f2d84ca (introduced in 3.9)
      bpo-28269: Replace strcasecmp with system function _stricmp. (pythonGH-13095)

Co-authored-by: David Sansome <[email protected]>
jcfr added a commit to python-cmake-buildsystem/cpython that referenced this pull request Jan 18, 2022
@jcfr @davidsansome
MinGW support: Add patches-win32/01-dynload_win.patch
743ac3e 
This commit integrates changes originally added to the project
as python-cmake-buildsystem/python-cmake-buildsystem@18d49757e (Mingw32 support, and add cmake
options for disabling)

Note that in  more recent version of Python, this patch is obsoleted
by the following commits:

  See python/cpython@589f89e2a (introduced in 3.3)
      Removed a Windows 9x trick used before LoadLibraryExW.
      Windows 9x has long been unsupported and the result of GetFullPathName was not even
      being used in the first place.

  See python/cpython@05f2d84ca (introduced in 3.9)
      bpo-28269: Replace strcasecmp with system function _stricmp. (pythonGH-13095)

Co-authored-by: David Sansome <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

+1 more reviewer

@BoboTiG BoboTiG BoboTiG approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gongminmin @serhiy-storchaka @skrah @miss-islington @bedevere-bot @BoboTiG @the-knights-who-say-ni