Skip to content

Use defined for PYBIND11_HAS_OPTIONAL, PYBIND11_HAS_EXP_OPTIONAL, PYBIND11_HAS_VARIANT #2476

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.

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

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

cirosantilli2
Copy link
Contributor

These variables are not set in certain circumstances, and if the project
using pybind11 sets -Wundef, the warnings will show.

In all other usages outside of stl.h, ifdef/defined has already been used,
this commit converts the remaining ones under stl.h.

The test build is also modified to catch the problem.

The errors looked like:

ext/pybind11/include/pybind11/stl.h:292:5: error: "PYBIND11_HAS_OPTIONAL" is not defined, evaluates to 0 [-Werror=undef]
  292 | #if PYBIND11_HAS_OPTIONAL
      |     ^~~~~~~~~~~~~~~~~~~~~
ext/pybind11/include/pybind11/stl.h:300:5: error: "PYBIND11_HAS_EXP_OPTIONAL" is not defined, evaluates to 0 [-Werror=undef]
  300 | #if PYBIND11_HAS_EXP_OPTIONAL
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
ext/pybind11/include/pybind11/stl.h:372:5: error: "PYBIND11_HAS_VARIANT" is not defined, evaluates to 0 [-Werror=undef]
  372 | #if PYBIND11_HAS_VARIANT
      |     ^~~~~~~~~~~~~~~~~~~~

@henryiii
Copy link
Collaborator

Note this changes the behavior; defining this to 0 causes it to be used now, instead of skipped. A completely backward compatible version would be #if defined(PYBIND11_HAS_OPTIONAL) && PYBIND11_HAS_OPTIONAL.

@YannickJadoul
Copy link
Collaborator

Note this changes the behavior; defining this to 0 causes it to be used now, instead of skipped. A completely backward compatible version would be #if defined(PYBIND11_HAS_OPTIONAL) && PYBIND11_HAS_OPTIONAL.

We're detecting and defining this ourselves, now, so... I don't think this is documented/meant to be set from the outside?

@henryiii
Copy link
Collaborator

We're detecting and defining this ourselves, now, so...

Probably okay, then. We need to change several #if (__APPLE__)'s to #if defined(__APPLE__) and such.

@YannickJadoul
Copy link
Collaborator

(Just wait for @bstaletic to pop up and shout about Hyrum's law, though.)

But another argument: pybind11's code detecting it also doesn't check if something is already defined:

#ifdef __has_include
// std::optional (but including it in c++14 mode isn't allowed)
# if defined(PYBIND11_CPP17) && __has_include(<optional>)
# include <optional>
# define PYBIND11_HAS_OPTIONAL 1
# endif
// std::experimental::optional (but not allowed in c++11 mode)
# if defined(PYBIND11_CPP14) && (__has_include(<experimental/optional>) && \
!__has_include(<optional>))
# include <experimental/optional>
# define PYBIND11_HAS_EXP_OPTIONAL 1
# endif
// std::variant
# if defined(PYBIND11_CPP17) && __has_include(<variant>)
# include <variant>
# define PYBIND11_HAS_VARIANT 1
# endif
#elif defined(_MSC_VER) && defined(PYBIND11_CPP17)
# include <optional>
# include <variant>
# define PYBIND11_HAS_OPTIONAL 1
# define PYBIND11_HAS_VARIANT 1
#endif

So if someone manually defines it, it's 1) overwrtiten (I think ?), and 2) likely gives some warning. So even considering a mild version of Hyrum's law, we should be fine? :-)

Would you mind having a look for the other macros that @henryiii mentions, and adding them to this PR, @cirosantilli2? :-)

@cirosantilli2
Copy link
Contributor Author

I've updated the PR to update __APPLE__ and __clang__ too, there however only one instance of those not using defined. Let me know if you can spot any others.

@henryiii
Copy link
Collaborator

Your warning addition should spot them 👍

@henryiii
Copy link
Collaborator

/__w/pybind11/pybind11/tests/test_stl.cpp:18:5: error: 'PYBIND11_HAS_VARIANT' is not defined, evaluates to 0 [-Werror,-Wundef]
57
#if PYBIND11_HAS_VARIANT

@YannickJadoul
Copy link
Collaborator

@cirosantilli2 If I'm not mistaken, warnings are treated as errors in the CI builds. So just follow the smell of the warnings/errors ;-)

The variables PYBIND11_HAS_OPTIONAL, PYBIND11_HAS_EXP_OPTIONAL, PYBIND11_HAS_VARIANT,
__clang__, __APPLE__ were not checked for defined in a minortity of instances.

If the project using pybind11 sets -Wundef, the warnings will show.

The test build is also modified to catch the problem.

The errors looked like:

```
ext/pybind11/include/pybind11/stl.h:292:5: error: "PYBIND11_HAS_OPTIONAL" is not defined, evaluates to 0 [-Werror=undef]
  292 | #if PYBIND11_HAS_OPTIONAL
      |     ^~~~~~~~~~~~~~~~~~~~~
ext/pybind11/include/pybind11/stl.h:300:5: error: "PYBIND11_HAS_EXP_OPTIONAL" is not defined, evaluates to 0 [-Werror=undef]
  300 | #if PYBIND11_HAS_EXP_OPTIONAL
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
ext/pybind11/include/pybind11/stl.h:372:5: error: "PYBIND11_HAS_VARIANT" is not defined, evaluates to 0 [-Werror=undef]
  372 | #if PYBIND11_HAS_VARIANT
      |     ^~~~~~~~~~~~~~~~~~~~
```
@henryiii henryiii merged commit b47efd3 into pybind:master Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants