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.

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

Resolve empty statement warning when using PYBIND11_OVERLOAD_PURE_NAME and PYBIND11_OVERLOAD_PURE #2325

Merged

Conversation

YannickJadoul
Copy link
Collaborator

Closes #2166

@YannickJadoul YannickJadoul requested review from rwgk and bstaletic July 24, 2020 22:43
@YannickJadoul YannickJadoul force-pushed the extra-semicolon-overload-macro branch from 4e080bd to cb51ef4 Compare July 24, 2020 22:51
do { \
PYBIND11_OVERLOAD_INT(PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), name, __VA_ARGS__); \
pybind11::pybind11_fail("Tried to call pure virtual function \"" PYBIND11_STRINGIFY(cname) "::" name "\""); \
} while (false)
Copy link
Contributor

Choose a reason for hiding this comment

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

The prior version of this macro ended with a ;, could break some users?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, removing these semicolons is the whole point of the PR, cfr. #2166!

But ... even if some user code breaks, in principle, 1. it's a loud breaking change (it won't silently crash, but it'll just complain during compilation), 2. usage is like that in all the docs and tests, so arguably, using the macro without ; is undocumented behavior.

include/pybind11/pybind11.h Show resolved Hide resolved
@bstaletic
Copy link
Collaborator

After a bit of sleep, I think the potential breaking change isn't worth it. The resolution to #2166 could as well be "remove the semicolon in user code".

@YannickJadoul
Copy link
Collaborator Author

After a bit of sleep, I think the potential breaking change isn't worth it. The resolution to #2166 could as well be "remove the semicolon in user code".

What made me remove it (rather than comments "well, then don't put the semicolon"), is that there's also a semicolon in the docs?

@bstaletic
Copy link
Collaborator

We can change the docs. Also #2166 isn't an error, just a warning raised by turning the -W settings up to 11.

@virtuald
Copy link
Contributor

virtuald commented Jul 25, 2020

Duplicate-ish of #2188

@YannickJadoul
Copy link
Collaborator Author

Duplicate-ish of #2188

Ah, I hadn't noticed. Just trying to fix/close some issues.

@wjakob
Copy link
Member

wjakob commented Jul 26, 2020

As long as we don't violate SemVer (i.e. documented behavior no longer compiling), I am okay with this change. It seems to me that there is no such violation (I only had a brief look though).

@YannickJadoul YannickJadoul self-assigned this Jul 26, 2020
@YannickJadoul YannickJadoul force-pushed the extra-semicolon-overload-macro branch 2 times, most recently from 6d32d02 to ab80ad1 Compare August 10, 2020 23:04
Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Kinds of breaking changes in this PR:

  • Moving functions from pybind11 to pybind11::detail namespace.
  • Renaming things.

I'm not strictly against these changes, however we need them explicitly enumerated in the upgrade guide.

include/pybind11/detail/internals.h Show resolved Hide resolved
include/pybind11/pybind11.h Show resolved Hide resolved
include/pybind11/pybind11.h Show resolved Hide resolved
do { \
PYBIND11_OVERRIDE_IMPL(PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), name, __VA_ARGS__); \
return cname::fn(__VA_ARGS__); \
} while (false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that a good change, the do { ... } while (false)?

Assuming we want to make this kind of a breaking change, sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is that ever going to break something that wasn't broken before, is the main question?
I can only imagine if (...) PYBIND11_OVERRIDE(...); to actually get fixed, rather than broken?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking of users using PYBIND11_OVERLOAD without a trailing semicolon. However, that's OVERLOAD, the deprecated macro. Moving on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, yes. I still need to fix that, such that the original macros are the same as they were. That was the whole point of this exercise!

@YannickJadoul
Copy link
Collaborator Author

YannickJadoul commented Aug 11, 2020

Right, so, I wanted to first still get the docs straight, but Boris is too fast for me :-)
I was planning to finish this PR and then give 3 options:

  1. We don't merge this one, but merge Function-like macro semicolon inconsistency fix #2188, and tell users to turn down their warnings (or even better, include pybind11 as system headers).
  2. We just merge the first commit (9230ac4), fixing Redundant semicolon in the definition of PYBIND11_OVERLOAD_PURE_NAME #2166. It would be breakish, but not according to the docs/SemVer (cfr. Resolve empty statement warning when using PYBIND11_OVERLOAD_PURE_NAME and PYBIND11_OVERLOAD_PURE #2325 (comment)).
  3. We merge this full PR, deprecating the old macros and functions, and finally getting the C++ terminology correct.

Your votes are welcome! :-)

My personal favourite is 3, obviously. Overloading is different from overriding. And it only gets worse, because we do have py::overload_cast and pybind11's overloading mechanism on functions. It can also get confusing when users try to mix: e.g. #2351.

To reply to @bstaletic:

Kinds of breaking changes in this PR:

* Moving functions from `pybind11` to `pybind11::detail` namespace.

Yes. As far as I know, just one (get_type_overload/detail::get_type_override), right?), undocumented, taking a parameter with a type in the detail namespace (detail::type_info), dealing with the internals/being an auxiliary function to a public-facing templated interface (get_overload/get_override), and I have created a deprecated alias outside of detail. If necessary, I can move it back out of detail. It just seemed like an oversight.

* Renaming things.

Yes. Though gently doing so, because I created deprecated aliases? Users' code should break. They should just get deprecation warnings.

I'm not strictly against these changes, however we need them explicitly enumerated in the upgrade guide.

Agreed. Planning on adding a big note in the docs :-)

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Your votes are welcome! :-)

My vote goes to whatever is least disruptive for users. I guess that makes it easy for me to vote against option 2. 1 and 3 seem to be about the same in this regard.

Moving functions from pybind11 to pybind11::detail namespace.

Yes. As far as I know, just one (get_type_overload/detail::get_type_override), right?)

Right.

undocumented, taking a parameter with a type in the detail namespace (detail::type_info), dealing with the internals/being an auxiliary function to a public-facing templated interface (get_overload/get_override),

I wouldn't be surprised if Hyrum's law rears its ugly head.

and I have created a deprecated alias outside of detail.

Oh, I didn't catch that! Nice.

If necessary, I can move it back out of detail. It just seemed like an oversight.

It does seem like an oversight. I guess we can move it into detail.

Renaming things.

Yes. Though gently doing so, because I created deprecated aliases? Users' code should break. They should just get deprecation warnings.

-Werror would promote the deprecation warning into an error, though I guess that's fine, because it's precisely what the user asked for.

include/pybind11/detail/internals.h Show resolved Hide resolved
include/pybind11/pybind11.h Show resolved Hide resolved
include/pybind11/pybind11.h Show resolved Hide resolved
do { \
PYBIND11_OVERRIDE_IMPL(PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), name, __VA_ARGS__); \
return cname::fn(__VA_ARGS__); \
} while (false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking of users using PYBIND11_OVERLOAD without a trailing semicolon. However, that's OVERLOAD, the deprecated macro. Moving on.

@bstaletic
Copy link
Collaborator

You forgot to update the docs/upgrade.rst. It's the actual "upgrade guide".

@YannickJadoul
Copy link
Collaborator Author

Alright, I'm waiting for #2370 before adding to the changelog and upgrade guide to avoid merge conflicts, but I believe this is ready for review!

To reiterate:

  1. We don't merge this one, but merge Function-like macro semicolon inconsistency fix #2188, and tell users to turn down their warnings (or even better, include pybind11 as system headers).

  2. We just merge the first commit (9230ac4), fixing Redundant semicolon in the definition of PYBIND11_OVERLOAD_PURE_NAME #2166. It would be breakish, but not according to the docs/SemVer (cfr. #2325 (comment)).

  3. We merge this full PR, deprecating the old macros and functions, and finally getting the C++ terminology correct.

Your votes are welcome! :-)

My personal favourite is 3, obviously. Overloading is different from overriding. And it only gets worse, because we do have py::overload_cast and pybind11's overloading mechanism on functions. It can also get confusing when users try to mix: e.g. #2351.

@YannickJadoul YannickJadoul force-pushed the extra-semicolon-overload-macro branch 2 times, most recently from f2100d9 to fc1c57f Compare August 11, 2020 12:02
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

My honest opinion: it is disproportionally disruptive to rename the macros, just to be semantically more correct. If we add a footnote to the documentation, explaining that override would be a more fitting name, but for historical reasons overload stuck, we're good.

My recommendation is to simply remove the semicolon at the end of the PYBIND11_OVERLOAD_PURE_NAME macro (as suggested under #2166) and mention it in the release notes. It's very easy to adjust to, therefore I think it's fine doing this in a patch release.

@YannickJadoul
Copy link
Collaborator Author

My honest opinion: it is disproportionally disruptive to rename the macros, just to be semantically more correct. If we add a footnote to the documentation, explaining that override would be a more fitting name, but for historical reasons overload stuck, we're good.

Yes, true, but then following that reasoning, such a mistake can never get fixed?
On the note of disruptive: users will only get a deprecation warning, though, which can be ignored? Everything will keep on working (until some pybind11 3.0.0 or so).

My recommendation is to simply remove the semicolon at the end of the PYBIND11_OVERLOAD_PURE_NAME macro (as suggested under #2166) and mention it in the release notes. It's very easy to adjust to, therefore I think it's fine doing this in a patch release.

But yes, this is fine with me as well, to just cherry-pick the first commit. Though @bstaletic was arguing that that would be more "breaking" than the deprecating PR...

@rwgk
Copy link
Collaborator

rwgk commented Aug 16, 2020

My honest opinion: it is disproportionally disruptive to rename the macros, just to be semantically more correct. If we add a footnote to the documentation, explaining that override would be a more fitting name, but for historical reasons overload stuck, we're good.

Yes, true, but then following that reasoning, such a mistake can never get fixed?
On the note of disruptive: users will only get a deprecation warning, though, which can be ignored? Everything will keep on working (until some pybind11 3.0.0 or so).

https://github.com/search?q=PYBIND11_OVERLOAD
I get 12k hits for "code".
I think it'll take at least a couple years until the old name is 90% flushed out of the global code bases. During that time there will be several hundred confused users, seeing the old name in some places, the new in others.
I agree it's a bit sad that this cannot easily be fixed, but I think the cost is too high. The old name is only "puzzling" but I don't think it rises to "misleading".
Please take my opinion only as one vote. If a majority supports the change I'll support it, too!

My recommendation is to simply remove the semicolon at the end of the PYBIND11_OVERLOAD_PURE_NAME macro (as suggested under #2166) and mention it in the release notes. It's very easy to adjust to, therefore I think it's fine doing this in a patch release.

But yes, this is fine with me as well, to just cherry-pick the first commit. Though @bstaletic was arguing that that would be more "breaking" than the deprecating PR...

I believe that'll trickle through the code bases much faster and decrease confusion monotonically, because the macros will be consistent.

@YannickJadoul
Copy link
Collaborator Author

https://github.com/search?q=PYBIND11_OVERLOAD
I get 12k hits for "code".

Alright, good point! Maybe we can provide a quick search-and-replace script to the upgrade guide.

At any rate, yes, the work is done, so now it's choosing an option. Probably up to @wjakob to decide, but let's see if we get more votes!

@wjakob
Copy link
Member

wjakob commented Sep 11, 2020

I took another look.

I really don't mind the name change, especially given that the previous naming scheme was arguably confusing/incorrect. It's also a good opportunity to clean up the inconsistency regarding semicolon usage.

What I don't like:

  1. This adds too much code, by a large margin. It should be possible to re-implement both variants of macros and functions with minimal duplication.

  2. I find it too early to annoy users with deprecation warnings, especially given the minor nature of the change. It's not like keeping around both variants is very costly in terms of the underlying implementation (c.f. point 1). My suggestion: let's only explain the new version in the docs, along with an explanation of the change in the upgrade guide, along with a note saying that the old version may be deprecated at some unspecified time.

@YannickJadoul
Copy link
Collaborator Author

Right, I'll have another look with that feedback in mind! :-)

@YannickJadoul YannickJadoul force-pushed the extra-semicolon-overload-macro branch from fc1c57f to 543d5a4 Compare September 14, 2020 18:24
@YannickJadoul
Copy link
Collaborator Author

(not sure why, but GitHub doesn't allow me to click the "re-request review" for you, @virtuald. But if you're interested, please do have a look :-) )

@virtuald
Copy link
Contributor

Changing the names seems like a reasonable compromise to me.

Copy link
Member

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

Looks great now, approved by me!

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@YannickJadoul YannickJadoul merged commit d65e34d into pybind:master Sep 15, 2020
@YannickJadoul
Copy link
Collaborator Author

Thanks all! Let's see how users react, on whether we need to update our upgrade guide further, or not :-)

@YannickJadoul YannickJadoul deleted the extra-semicolon-overload-macro branch September 15, 2020 12:57
lanctot pushed a commit to google-deepmind/open_spiel that referenced this pull request Mar 27, 2023
`PYBIND11_OVERRIDE_IMPL` is an implementation detail and not meant to be used outside the pybind11 source tree.

Notes:

* For easy reference: `PYBIND11_OVERRIDE_IMPL` was added to python_games.cc in cl/379240506.

* Discovered in connection with google/pybind11clif#30015, which changes `PYBIND11_OVERRIDE_IMPL`. (The pywrapcc fork of pybind11 is not currently used in production, but we're using it regularly to run TAP Global Presubmits, to test go/pyclif_pybind11_fusion developments.)

* FYI: Test coverage seems to be incomplete: control flow passes through the changed code for many tests, but replacing `"mean_field_population"` with `"XXXmean_field_population"` does not break any tests (see isolated TGP results under http://tap/OCL:517999370:BASE:517998831:1679332509158:a5f7412a). I.e. a test with a Python override is missing.

* FYI: The `PYBIND11_OVERLOAD_*` macros used in other parts of python_games.cc are deprecated since Sep 2020 (pybind/pybind11#2325). It is recommended to replace them with the equivalent `PYBIND11_OVERRIDE_*` macros.

PiperOrigin-RevId: 518266312
Change-Id: Ica90764a71b8ed7795b2f26ab02a904bf2ad901e
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.

Redundant semicolon in the definition of PYBIND11_OVERLOAD_PURE_NAME
5 participants