Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving pragma for MSVC warning C4505 from pybind11.h to existing list in detail/common.h #3160

Merged
merged 3 commits into from
Jul 30, 2021
Merged

Moving pragma for MSVC warning C4505 from pybind11.h to existing list in detail/common.h #3160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9d124b
Moving pragma for C4505 from pybind11.h to existing list in detail/co…
Jul 30, 2021
9627e55
Removing 4 existing suppressions to 1. see what is still needed and 2…
Jul 30, 2021
a3f1c3d
It turns out none of the 4 pragmas are needed anymore.
Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@
# define HAVE_ROUND 1
# endif
# pragma warning(push)
# pragma warning(disable: 4510 4610 4512 4005)
// C4505: 'PySlice_GetIndicesEx': unreferenced local function has been removed (PyPy only)
# pragma warning(disable: 4505)
# if defined(_DEBUG) && !defined(Py_DEBUG)
# define PYBIND11_DEBUG_MARKER
# undef _DEBUG
Expand Down
1 change: 0 additions & 1 deletion include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#if defined(_MSC_VER) && !defined(__INTEL_COMPILER)
# pragma warning(push)
# pragma warning(disable: 4127) // warning C4127: Conditional expression is constant
# pragma warning(disable: 4505) // warning C4505: 'PySlice_GetIndicesEx': unreferenced local function has been removed (PyPy only)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like that we lose the comment here, especially the PyPy only part. Could we move the comment to just above the other place? Also, we could make this PyPy only, if we wanted to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll copy it over to the other place. It will look odd though as the only one. What do you think about adding the message for all diagnostics, for completeness?

Also, we could make this PyPy only, if we wanted to.

I was thinking about that but

  • how do we know that we have PyPy before including Python.h?

  • how much do we want to, given 1. that there are four other diagnostics already, 2. how useful is the warning, really? The suppression is not even for our code.

But in any case, if there is a practical method to make it PyPy only, I'd be happy to do it that way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about adding the message for all diagnostics, for completeness?

+1, if you want to do it :)

how do we know that we have PyPy before including Python.h?

This is probably why it's there without a check for PyPy currently. My only worry is that, like hiding any warning, you are also are losing sensitivity to what it was supposed to catch as well. Perfectly fine with not making it depend on PyPy then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@henryiii please take another look: it turns out none of the existing four suppressions is needed anymore.

#elif defined(__GNUG__) && !defined(__clang__) && !defined(__INTEL_COMPILER)
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wunused-but-set-parameter"
Expand Down