-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Make PYBIND11_MODULE name usable with define #1082
Conversation
include/pybind11/detail/common.h
Outdated
@@ -205,6 +205,7 @@ extern "C" { | |||
#define PYBIND11_TRY_NEXT_OVERLOAD ((PyObject *) 1) // special failure return code | |||
#define PYBIND11_STRINGIFY(x) #x | |||
#define PYBIND11_TOSTRING(x) PYBIND11_STRINGIFY(x) | |||
#define PYBIND11_STRCONCAT(first, second) first##second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: this isn't actually string concatenation; perhaps rename to just PYBIND11_CONCAT
?
include/pybind11/detail/common.h
Outdated
#define PYBIND11_MODULE(name, variable) \ | ||
static void pybind11_init_##name(pybind11::module &); \ | ||
static void PYBIND11_STRCONCAT(pybind11_init_,name)(pybind11::module &); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style consistency issue: need a space after a comma.
include/pybind11/detail/common.h
Outdated
@@ -293,7 +295,7 @@ extern "C" { | |||
return nullptr; \ | |||
} \ | |||
} \ | |||
void pybind11_init_##name(pybind11::module &variable) | |||
void PYBIND11_STRCONCAT(pybind11_init_,name)(pybind11::module &variable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me; I left a couple comments for minor cosmetic details.
This is a pretty small change and seems worth including in 2.2.1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I just have two whitespace nitpicks.
include/pybind11/detail/common.h
Outdated
@@ -266,8 +267,9 @@ extern "C" { | |||
}); | |||
} | |||
\endrst */ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove whitespace between docs and definition.
include/pybind11/embed.h
Outdated
@@ -43,12 +43,14 @@ | |||
}); | |||
} | |||
\endrst */ | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Merged, thanks! |
The name in the
PYBIND11_MODULE(name, variable)
#define can not be used with a preprocessor define.Given a simple cpp file
define_module_name.cpp
, whereMODULENAME
is defined elsewhere, and preprocess it.The result in
define_module_name.ii
will contain bothmy_module_name
andMODULENAME
:This commit fixes this by adding one level of macro indirection.