Skip to content

Function-like macro semicolon inconsistency fix #2188

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

Closed
wants to merge 1 commit into from
Closed

Function-like macro semicolon inconsistency fix #2188

wants to merge 1 commit into from

Conversation

Ricardicus
Copy link
Contributor

@Ricardicus Ricardicus commented Apr 27, 2020

I was using pybind11 to wrap a class containing virtual and pure virtual functions.

I noticed that the function-like macros: PYBIND11_OVERLOAD and PYBIND11_OVERLOAD_PURE behaved a bit differently when I was compiling my program.

I ran into errors where the compiler complained that I was missing a semicolon when using PYBIND11_OVERLOAD but it did not complain when I was using PYBIND11_OVERLOAD_PURE.
So there is an inconsistency here making PYBIND11_OVERLOAD_PURE statements ok without semicolons.

What I am trying to say in code:

/* Compiler thinks this is fine */
PYBIND11_OVERLOAD_PURE( .... )
PYBIND11_OVERLOAD_PURE( .... )

/* Compiler does not think this is fine */
PYBIND11_OVERLOAD( .... )
PYBIND11_OVERLOAD( .... )

/* I must add semicolons to make it happy */
PYBIND11_OVERLOAD( .... );
PYBIND11_OVERLOAD( .... );

There is a slight inconsistency here.

I introduced a change so that PYBIND11_OVERLOAD can be called with or without semicolons and I thought perhaps maybe you would like to see it also.

@Ricardicus
Copy link
Contributor Author

One can go two directions here; either force the semicolon to be added or let it be optional.

However since the _PURE macro has no need for a semicolon to end it, I went with making the other optional just for backward compatibilty, maybe there are projects out there with PYBIND11_OVERLOAD_PURE statements without semicolon...

@Ricardicus
Copy link
Contributor Author

Ricardicus commented Sep 17, 2020

I see you have fixed this now 👍

@Ricardicus Ricardicus closed this Sep 17, 2020
@YannickJadoul
Copy link
Collaborator

Thanks for the PR, though, @Ricardicus! It was decided to fix the naming and make the semicolons mandatory, while we're at it :-)

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.

2 participants