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

Jump to bottom

Removing last remnants of pragma block at the top of pybind11.h #3186

Merged
merged 17 commits into from
Aug 14, 2021
Merged

Removing last remnants of pragma block at the top of pybind11.h #3186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
66b62f1
Removing last remnants of pragma block at the top of pybind11.h, defa…
Aug 9, 2021
c75c325
Unique SOSIZE prefix to make it easier to extract the sosizes from th…
Aug 11, 2021
5a7bd10
Commenting out PYBIND11_WERROR block, for noinline testing.
Aug 11, 2021
aca0e20
Undoing accidental change.
Aug 11, 2021
d760646
`#define PYBIND11_NOINLINE_FORCED`
Aug 11, 2021
4e57e6a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 11, 2021
bfa47b3
`#define PYBIND11_NOINLINE_DISABLED`
Aug 11, 2021
114cf66
Going back to default (removing `#define PYBIND11_NOINLINE_DISABLED`).
Aug 12, 2021
e043cca
`#define PYBIND11_NOINLINE_FORCED`
Aug 12, 2021
12600e8
Undoing all changes releated to measuring sosizes.
Aug 12, 2021
86bb2a8
Rollback of PR #3030 (Working around Centos 8 failure).
Aug 12, 2021
4927293
Disabling -Werror for GNU (experiment).
Aug 12, 2021
4bd03f2
Commenting out the entire `if(PYBIND11_WERROR)` again (although that …
Aug 12, 2021
027aa9f
Adding `-DCMAKE_BUILD_TYPE=Release`
Aug 12, 2021
a4dbe3c
Undoing change to tests/CMakeLists.txt (uncommenting `if(PYBIND11_WER…
Aug 12, 2021
3457185
post `git rebase master -X theirs` fixups.
Aug 12, 2021
fa53ba5
Adding measurements to comment for `PYBIND11_NOINLINE_FORCED`.
Aug 12, 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
11 changes: 11 additions & 0 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@
# endif
#endif

// For CUDA, GCC7, GCC8:
// PYBIND11_NOINLINE_FORCED is incompatible with `-Wattributes -Werror`.
// When defining PYBIND11_NOINLINE_FORCED, it is best to also use `-Wno-attributes`.
// However, the measured shared-library size saving when using noinline are only
// 1.7% for CUDA, -0.2% for GCC7, and 0.0% for GCC8 (using -DCMAKE_BUILD_TYPE=MinSizeRel,
// the default under pybind11/tests).
#if !defined(PYBIND11_NOINLINE_FORCED) && \
(defined(__CUDACC__) || (defined(__GNUC__) && (__GNUC__ == 7 || __GNUC__ == 8)))
# define PYBIND11_NOINLINE_DISABLED
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am guessing there is no clean way to detect if the warning is enabled is there? If there is seems like it might make sense to add it to the conditional.

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 am guessing there is no clean way to detect if the warning is enabled is there?

I don't think so, not even noinline is portable, even less so introspection of compiler-specific warnings related to it.

If there is seems like it might make sense to add it to the conditional.

If it was easy, yes.

But how likely is it that anyone cares enough about a 1.7% binary size reduction when building with CUDA? If someone does, they can add the suggested command line options. If they don't like that, they could send us a PR. — The group of affected people and the binary size saving both seem so marginal to me, I cannot imagine it's worth anyone's time, cost vs benefit.

#endif

// The PYBIND11_NOINLINE macro is for function DEFINITIONS.
// In contrast, FORWARD DECLARATIONS should never use this macro:
// https://stackoverflow.com/questions/9317473/forward-declaration-of-inline-functions
Expand Down
9 changes: 0 additions & 9 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@

#pragma once

#if defined(__CUDACC__) || (defined(__GNUC__) && (__GNUC__ == 7 || __GNUC__ == 8))
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wattributes"
#endif

#include "attr.h"
#include "gil.h"
#include "options.h"
Expand Down Expand Up @@ -2383,7 +2378,3 @@ PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
#if defined(__GNUC__) && __GNUC__ == 7
# pragma GCC diagnostic pop // -Wnoexcept-type
#endif

#if defined(__CUDACC__) || (defined(__GNUC__) && (__GNUC__ == 7 || __GNUC__ == 8))
# pragma GCC diagnostic pop
#endif