Removing MSVC C4996 from pragma block at the top of pybind11.h #3129
Conversation
|
I'm having second thoughts after review of this spreadsheet: pybind11.h v2.7.0 pragma block analysis Marking this PR as draft for now. |
include/pybind11/pybind11.h
Outdated
| @@ -56,6 +57,21 @@ | |||
|
|
|||
| PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) | |||
|
|
|||
| PYBIND11_NAMESPACE_BEGIN(detail) | |||
|
|
|||
| #if defined(_MSC_VER) | |||
There was a problem hiding this comment.
Instead of poping the warning, I think the recommended way to handle this error is actually to call the _strdup on MSC: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strdup-wcsdup?view=msvc-160 strdup is an alias to it.
There was a problem hiding this comment.
The 2nd thoughts I had when marking this PR as draft yesterday:
- Cluttering pybind11 with MSVC-specific things is ugly.
- Better to simply disable the warning on the command line?
To me it looks like just another MSVC oddity, dirt that I want to keep out of my house, to use stronger words.
But we have similar pros-and-cons for most of the 11 items that I enumerated here yesterday:
pybind11.h v2.7.0 pragma block analysis
I was thinking of suggesting a focused action meeting to go through the list and decide what to do about each one. I'll ping you and @henryiii on slack about it.
There was a problem hiding this comment.
I think we should do what MSVC wants; like it or not, they are a major target for us. We should not requires any special flags to build without warnings; this is a header-only library and is not always called through build methods we control.
rwgk
force-pushed
the
pragma_block_rm_msvc_c4996
branch
from
267f86c to
2462d28
Compare
|
@henryiii @Skylion007 Please take another look. The latest version is completely different compared to the initial one. |
|
Thanks! I'll merge this now, then rebase the other PRs. |
… __STDC_LIB_EXT1__ is not defined by the implementation. Follow-on to PR pybind#3129.
…d#3129) * Removing MSVC C4996 from pragma block at the top of pybind11.h * localtime_thread_safe, PYBIND11_COMPAT_STRDUP * Adding #include <ctime> (attempt to fix MSVC 2015, 2017 errors).
… __STDC_LIB_EXT1__ is not defined by the implementation. (pybind#3151) Follow-on to PR pybind#3129.
Follow-on to PR #3127, based on results obtained under PR #3125.
The suggested changelog entry will be in the final PR of this cleanup series.