[Many pragma] Removing last remnants of pragma block at the top of pybind11.h

[rwgk]

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.

Worksheet

---

This PR changes 9 files, removes 14 lines, adds 92 lines.

---

Most significant observation:

For all compilers except {CUDA, GCC7, GCC8, ICC}, a correct way to specify a forward declaration for a noinline function is:

int foo(); // NO inline here!

#if defined(_MSC_VER)
inline __declspec(noinline) foo() { return 0; }
#else
inline __attribute__ ((noinline)) foo() { return 0; }
#endif

There are no warnings, #pragma GCC diagnostic ignored "-Wattributes" is NOT needed:
https://github.com/pybind/pybind11/actions/runs/1091508338

See also: https://stackoverflow.com/questions/9317473/forward-declaration-of-inline-functions:
The stackoverflow page essentially suggests that the above is the one correct way to specify a forward declaration.

For {CUDA, GCC7, GCC8} our options are:

• Use PYBIND11_NOINLINE_DISABLED (see detail/common.h).
• Or add pragmas.

Adding pragma around just the forward declarations is insufficient:
FAILING https://github.com/pybind/pybind11/actions/runs/1091968458

The pragmas need to be added in 9 header files: (4 + 3) x 9 = 73 lines + 18 blank lines = 81 lines.
The 9 pragmas work: SUCCESS https://github.com/pybind/pybind11/actions/runs/1092076877

In summary, we have a choice between cluttering our code with 9 x 2 small pragma blocks, or using PYBIND11_NOINLINE_DISABLED for the few troublesome platforms, while also providing PYBIND11_NOINLINE_FORCED as an option. See the associated comment in detail/common.h.

Note that the 9 x 2 pragmas are a liability when refactoring. Depending on how code is moved, some may not be needed anymore (something that is likely to be overlooked), or they need to be copied to more files.

---

For completeness:

The ICC pragma 2196 cannot be removed:
FAILING https://github.com/pybind/pybind11/actions/runs/1092275777

---

Pointing out a special case:

Converted from inline to PYBIND11_NOINLINE for consistency:

pybind11.h:inline void keep_alive_impl(handle nurse, handle patient) {

---

Note that this PR adds inline to the PYBIND11_NOINLINE macro, which simplifies code elsewhere and removes the potential for inconsistencies (inconsequential, just not nice):

inline first:

numpy.h:inline PYBIND11_NOINLINE void load_numpy_internals(numpy_internals* &ptr) {
numpy.h:inline PYBIND11_NOINLINE void register_structured_dtype(
detail/internals.h:inline PYBIND11_NOINLINE void *get_shared_data(const std::string &name) {
detail/internals.h:inline PYBIND11_NOINLINE void *set_shared_data(const std::string &name, void *data) {

inline second:

pybind11.h:PYBIND11_NOINLINE inline void keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret) {
pybind11.h:PYBIND11_NOINLINE inline void print(const tuple &args, const dict &kwargs) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline void all_type_info_populate(PyTypeObject *t, std::vector<type_info *> &bases) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline detail::type_info* get_type_info(PyTypeObject *type) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline detail::type_info *get_type_info(const std::type_index &tp,
detail/type_caster_base.h:PYBIND11_NOINLINE inline handle get_type_handle(const std::type_info &tp, bool throw_if_missing) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline handle find_registered_python_instance(void *src,
detail/type_caster_base.h:PYBIND11_NOINLINE inline value_and_holder instance::get_value_and_holder(const type_info *find_type /*= nullptr default in common.h*/, bool throw_if_missing /*= true in common.h*/) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline void instance::allocate_layout() {
detail/type_caster_base.h:PYBIND11_NOINLINE inline void instance::deallocate_layout() const {
detail/type_caster_base.h:PYBIND11_NOINLINE inline bool isinstance_generic(handle obj, const std::type_info &tp) {
detail/type_caster_base.h:PYBIND11_NOINLINE inline std::string error_string() {
detail/type_caster_base.h:PYBIND11_NOINLINE inline handle get_object_handle(const void *ptr, const detail::type_info *type ) {
detail/common.h:[[noreturn]] PYBIND11_NOINLINE inline void pybind11_fail(const char *reason) { throw std::runtime_error(reason); }
detail/common.h:[[noreturn]] PYBIND11_NOINLINE inline void pybind11_fail(const std::string &reason) { throw std::runtime_error(reason); }
detail/typeid.h:PYBIND11_NOINLINE inline void clean_type_id(std::string &name) {
detail/internals.h:PYBIND11_NOINLINE inline internals &get_internals() {

[rwgk]

@henryiii @Skylion007
I just discovered this: https://stackoverflow.com/questions/9317473/forward-declaration-of-inline-functions
I mistakingly thought the inline has to appear in the forward declaration.
Based on the stackoverflow information the PYBIND11_NOINLINE_FWD macro doesn't make sense. I'll remove it.

Putting this PR back in Draft mode. I'll ping you when I'm done updating.

[rwgk]

Follow-on question to the discovery of https://stackoverflow.com/questions/9317473/forward-declaration-of-inline-functions:

If it is true that forward declarations never need inline, the ICC and GCC warnings do not seem to make any sense at all!?

Isn't what we really want a compiler warning? Something like: Superfluous 'inline' in forward declaration.

I looked for inline and attr in the list of clang-tidy checks but there is nothing:

https://clang.llvm.org/extra/clang-tidy/checks/list.html

[rwgk]

@henryiii @Skylion007 The PR description is now significantly simpler. Could you please take a look? — I'm hoping for agreement to purge the 9 x 2 pragma blocks from this PR.

[henryiii]

We can see what @Skylion007 (or anyone else) says, but I'm in favor of the pragmas. Leaking warning suppression into user code is a clear regression, not an improvement. Having "secret" defines that force enable or disable things is ugly. And then there's still no way for a user to have attribute warnings enforced on their code as errors, since pybind11 is either hiding them or erroring on them. I think this is just an unavoidable issue caused by forcing ourselves to be header-only. I'm not so strongly in favor of the pragmas that I can't be convinced otherwise if others disagree, though. (And correct me if I'm misunderstanding the suggestion).

I think, when we work on the pre-compilation step, this might become much simpler. All the "definition" files could be surrounded by this pragma as part of the standard setup/teardown for those files - after all, all NOINLINE functions will be pre-compilable and will not have a the no inline setting active when in pre-compile mode.

[rwgk]

My thinking is:

The only "loss" going the route withOUT the pragmas is:

• If and only if you have gcc 7, gcc 8, or cuda: you can either have noinline or -Wattributes, but not both.

Side remark: potentially we can check the cuda version and modern versions may be fine.

Contrast that with: developers have to permanently work with and around 9 x 2 pragma blocks, just so a few select platforms can activate a warning that they most likely get from also testing with other platforms anyway.

Additionally: do we have performance or binary size measurements that would justify prioritizing

• noinline + -Wattributes on a few select platforms
  over
• littering our code with pragmas?

Net conclusion I'm arriving at (without having such data):

• with the pragmas we are certainly losing,
• while it's very uncertain that anyone is gaining anything significant.

We will not leak warnings, it's only that a small fraction of pybind11 users will have to make the either-or choice above.

@henryiii @Skylion007 could this sway your vote?

[rwgk]

I converted this back to Draft for the minute.
The CI run is just to retest after git rebase master (included manually resolving conflicts).
My plan is to break this up into two PRs, so that it will be much more obvious how the alternatives pan out.
Probably tomorrow.

[rwgk]

I decided it is best to split out PR #3179, get that out of the way first, then revisit the pragma vs either-or-for-a-few-platforms question. It will be much easier to see what the alternatives entail.

[rwgk]

Alternative PR #3186 was just merged. Discarding this PR.