src: add CURL_STRICMP() macro, use _stricmp() on Windows
#15788
Conversation
Anything else is a deprecated alias for it. https://learn.microsoft.com/cpp/c-runtime-library/reference/stricmp-wcsicmp https://learn.microsoft.com/cpp/c-runtime-library/reference/strcmpi -> https://learn.microsoft.com/cpp/c-runtime-library/reference/stricmp-wcsicmp-mbsicmp-stricmp-l-wcsicmp-l-mbsicmp-l
src/tool_util.c
Outdated
| @@ -181,7 +181,9 @@ int struplocompare(const char *p1, const char *p2) | |||
| return p2 ? -1 : 0; | |||
| if(!p2) | |||
| return 1; | |||
| #ifdef HAVE_STRCASECMP | |||
| #if defined(_WIN32) | |||
There was a problem hiding this comment.
It seems clearer to readers to use #ifdef HAVE_STRICMP here. Then we don't need to know that this is Windows specific when reading the code.
There was a problem hiding this comment.
As a policy, we check for features in ifdefs rather than OSes etc so that when something else at some point starts to use this feature it will just automatically work.
There was a problem hiding this comment.
On Windows stricmp is a deprecated function. Depending on
toolchain, at the moment one of it's many synonyms are detected
and used, sometimes wrongly. _stricmp is the Windows-specific
name that's always guarenteed to exist.
We already do _WIN32 checks at a lot of similar places. I'm not
sure I understand the issue for this specific case.
Should I detect _stricmp with all the dance at build-level, just to
have HAVE__STRICMP here? That would at best wasting configure
time and adding complexity, unless there ever comes up a non-Windows
platform which uses _stricmp. (Apple was one of these where it
turned out to be an elaborate upstream bug causing mis-detections:
8b76a8a #15526
93e6e4b #15559)
There was a problem hiding this comment.
I generally prefer us doing ifdef [feature] even if that feature is a unique feature for a single operating system, rather than ifdef [operatingsystem].
I know we do win32 checks in many places, but that is generally the wrong approach and we should strive towards avoiding them when we can easily do so.
There was a problem hiding this comment.
Or perhaps add a global CURL_STRICMP() that maps to the
correct implementation at a single place? And use that in
struplocompare() and other, future, places?
There was a problem hiding this comment.
We don't have to "detect" the feature any more than we detect other windows-specific functions. We can just say "if windows", set this define. Like we do for many other features.
There was a problem hiding this comment.
Or perhaps add a global CURL_STRICMP() that maps to the correct implementation at a single place?
Sounds like a good idea
There was a problem hiding this comment.
Generally I agree, feature-check is peferred to OS-check.
But, I'm not sure this helps this particular case, where
feature is exclusive to operatingsystem.
And the feature is one of several detected alternate options, so
checking order becomes significant without the code telling this
story. Plus the added busywork up the chain.
It seems clearer and simpler to shortcut _WIN32 in this case.
There was a problem hiding this comment.
The macro way works for me.
There was a problem hiding this comment.
I'm not sure this helps this particular case, where feature is exclusive to operatingsystem.
The things is that when done correctly, we don't know that it is unique to a single OS and we don't have to care. When we want to build curl on the new OS ABC tomorrow that has this function, having done the code correctly based on features then makes it an easier ride.
vszakats
changed the title
_stricmp()CURL_STRICMP() macro, use _stricmp() on Windows
Add
CURL_STRICMP()macro that works on all platforms depending onwhich lib C function is available.
Make sure to always use
_stricmp()on Windows, which is thenon-deprecated, official API for this on this platform. Before this
patch it used a MinGW-specific call, or a deprecated compatibility
wrapper with MSVC.
Drop
stricmpvariant detections on Windows with autotools.https://learn.microsoft.com/cpp/c-runtime-library/reference/stricmp-wcsicmp-mbsicmp-stricmp-l-wcsicmp-l-mbsicmp-l
Ref: #15652