Change NAMESPACE_BEGIN and NAMESPACE_END macros into PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END #2283

YannickJadoul · 2020-07-08T12:16:07Z

Straightforward PR, implementing suggestion from #2199.

Closes #2199.

…CE_BEGIN and PYBIND11_NAMESPACE_END

wjakob · 2020-07-08T12:19:52Z

This is a no-brainer, feel free to merge once the CI passes.

bstaletic · 2020-07-08T12:43:09Z

So CI is broken. Partially because PyPy is broken since yesterday, like we discussed.

The other error is:

+brew upgrade python
Error: python not installed

YannickJadoul · 2020-07-08T12:48:28Z

The other error is:

+brew upgrade python
Error: python not installed

Sigh. How does Travis keep on changing what's default installed with homebrew??

.travis.yml

…__init__' error message between CPython and PyPy under the rug

bstaletic · 2020-07-08T19:38:57Z

Messages from brew install python:

==> Caveats
Python has been installed as
  /usr/local/opt/[email protected]/bin/python3
You can install Python packages with
  /usr/local/opt/[email protected]/bin/pip3 install <package>
They will install into the site-package directory
  /usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages

So it installed python 3.8, not 3.7. This could explain the error:

ld: library not found for -lpython3.7m

henryiii · 2020-07-08T19:44:29Z

Homebrew finally finished the Python 3.7 to 3.8 transition, was merged yesterday, so this isn't really Travis' fault.

YannickJadoul · 2020-07-08T19:47:25Z

Thanks, @bstaletic and @henryiii. Not behind my laptop, currently. Feel free to push a fix to my branch!

YannickJadoul · 2020-07-08T19:55:31Z

.travis.yml

@@ -149,7 +149,7 @@ matrix:
  - os: osx
    name: Python 3.7, c++14, AppleClang 9, Debug build


Description is still 3.7

How did I miss that? I'll have to checkout & force push now...

I can do it in a few hours as well!

YannickJadoul · 2020-07-08T20:00:06Z

Thanks Henry!

Something fishy still happened before, btw, because CMake was called with -DPYBIND11_PYTHON_VERSION=3.7 and reported

-- Found PythonInterp: /usr/local/bin/python3.7 (found suitable version "3.7.5", minimum required is "3.7") 
-- Found PythonLibs: python3.7m

That's why I didn't look further at HomeBrew's installation...

henryiii · 2020-07-08T20:05:16Z

python became [email protected], [email protected] became python and was allowed to override the links in /usr/local/. It looks to me that 3.7 didn't become keg-only, though I rather would have thought it should have. python3.7 is still installed and still available.

YannickJadoul · 2020-07-08T20:37:54Z

Ah, right, thanks for the explanation! I don't know brew's thát well :)

bstaletic · 2020-07-08T20:58:36Z

Someone could cancel the second to last appveyor run and save this PR ~20 minutes.

YannickJadoul · 2020-07-08T21:02:10Z

Appveyor doesn't give collaborators access. It's on wjakob's account and we'd need an explicit invitation (which is probably not worth the trouble if we're switching to GHA soon enough).

henryiii · 2020-07-08T22:15:11Z

@wjakob already approved, so merged. Thanks!

YannickJadoul · 2020-07-08T22:18:32Z

3 minutes too late to merge! Thanks for the final fixes, @henryiii!

patrikhuber · 2020-08-01T15:12:18Z

Hi,

I updated from an older pybind to the recent master today and was greeted with a bunch of compiler error messages. If I see it correctly, this is a change that came after the 2.5.0 release so I'm probably ahead of the curve. I couldn't find anything in the docs about it. I would strongly recommend to mention this in the Upgrade Guide https://pybind11.readthedocs.io/en/stable/upgrade.html.

Also the typecaster example in the docs https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html uses the manual namespace pybind11 { namespace detail { and not the macro. In fact, a search in the documentation for NAMESPACE_BEGIN or PYBIND11_NAMESPACE_BEGIN yields no (useful) results. I would recommend adding the macro to the docs and in particular to the custom type caster example?

Thank you all for working on and improving pybind11, it's one of the most awesome libraries/tools out there! :-)

patrikhuber · 2020-08-01T15:21:23Z

I'm actually now wondering what this macro is for - as you still need to specify the actual name of the namespace, e.g.:

PYBIND11_NAMESPACE_BEGIN(pybind11)
PYBIND11_NAMESPACE_BEGIN(detail)

I can't see any benefits? It's not shorter (longer even) and you still need to remember and "manually" type the name of the actual namespace, so you might as well just type

namespace pybind11 {
namespace detail {

It would be useful if you could just do:

PYBIND11_NAMESPACE_BEGIN
// ... my typecaster code ...
PYBIND11_NAMESPACE_END

In that way, as a user/consumer of pybind11 I don't have to remember what namespace to define my stuff in, and if you pybind11 folks ever decide to change the namespace, my code is future proof. I originally thought that's what the purpose of the macro is, but now see that that's currently not the case.

YannickJadoul · 2020-08-01T15:27:17Z

This is an undocumented macro, though, so I'm not sure pybind11 is responsible for breaking undocumented stuff? ;-) (Then again, I also don't really know why the macros are actually there?)

There's also not a new release yet, and I don't think it's clear yet whether it will be a patch, minor, or major release, so it's hard to write this part of the docs (and anyway, it won't show up on the "stable" version of RTD!). Though I guess it could be a good plan to keep a list of changes to master, there (@henryiii did start a "pending" changelog, I believe).

I can't see any benefits?

Not sure why it's there, but there must be a reason?

It would be useful if you could just do:
PYBIND11_NAMESPACE_BEGIN
// ... my typecaster code ...
PYBIND11_NAMESPACE_END

That wouldn't really make sense, because it's in pybind11::detail, and adding detail by defaults will give other problems (because obviously, some things (actually, all "public" things) are nót in detail. The casters are the weird one out because you are allowed to create your own caster, but I believe pybind11 doesn't guarantee that the interface won't break, so that (and historical reasons) is why it's in detail.

PS: Expecting @bstaletic to pop up any time now, shouting "Hyrum's law".

bstaletic · 2020-08-01T15:50:29Z

A few thoughts.

NAMESPACE_BEGIN and NAMESPACE_END are horrible macro names that should never appear in any code base.
The docs don't mention it because there's no reason to.
The docs use namespace pybind11 { namespace detail { because that's what is officially supported.
Does pybind11 have users who depend on the exact definition of header guards?

I'm honestly surprised that anyone would try to depend on something like NAMESPACE_BEGIN. As it just doesn't come up in the docs or issues, except in some diffs, one would almost have to look for these macros and use something out of contract.

That said...

PS: Expecting @bstaletic to pop up any time now, shouting "Hyrum's law".

Exactly!

henryiii · 2020-08-01T16:01:37Z

Hyrum's law

That's exactly what I thought of when I saw this. :)

henryiii · 2020-08-01T16:03:05Z

I didn't remember the upgrade guide, thanks! Will make sure CMake related things go in there soon!

patrikhuber · 2020-08-02T07:31:07Z

I see! Makes complete sense. I agree then it should not be mentioned in the docs or upgrade guide, as it's internal-use only. I've removed it from my type-casters and changed it to namespace pybind11 { namespace detail {.

I don't remember why I used that macro or where I found it, but I don't think I was specifically looking for it. If I had to venture a guess I would say that it (mistakenly, in that case) might have been part of some pybind11 example typecaster code somewhere a few years ago, and I copied it from there. But anything is possible.

Thanks a lot, and I guess "sorry for using undocumented stuff" :-).

YannickJadoul · 2020-08-02T07:52:58Z

Thanks for the friendly reminder that we need to start thinking about this upgrade guide ;-)

Change NAMESPACE_BEGIN and NAMESPACE_END macros into PYBIND11_NAMESPA…

e979d0a

…CE_BEGIN and PYBIND11_NAMESPACE_END

YannickJadoul mentioned this pull request Jul 8, 2020

NAMESPACE_BEGIN macro redefinition #2199

Closed

YannickJadoul closed this Jul 8, 2020

YannickJadoul reopened this Jul 8, 2020

bstaletic reviewed Jul 8, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

YannickJadoul added 2 commits July 8, 2020 17:05

Fix sudden HomeBrew 'python not installed' error

3096e96

Sweep difference in 'Class.__init__() must be called when overriding …

7a8cc07

…__init__' error message between CPython and PyPy under the rug

YannickJadoul force-pushed the fix-namespace-macro-collisions branch from b33874c to 7a8cc07 Compare July 8, 2020 15:05

YannickJadoul commented Jul 8, 2020

View reviewed changes

henryiii force-pushed the fix-namespace-macro-collisions branch from 84eca63 to 7c16b80 Compare July 8, 2020 20:17

Homebrew updated to 3.8 yesterday.

681ebc4

henryiii force-pushed the fix-namespace-macro-collisions branch from 7c16b80 to 681ebc4 Compare July 8, 2020 20:17

henryiii approved these changes Jul 8, 2020

View reviewed changes

henryiii merged commit f980d76 into pybind:master Jul 8, 2020

YannickJadoul deleted the fix-namespace-macro-collisions branch July 8, 2020 22:17

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change NAMESPACE_BEGIN and NAMESPACE_END macros into PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END #2283

Change NAMESPACE_BEGIN and NAMESPACE_END macros into PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END #2283

YannickJadoul commented Jul 8, 2020

wjakob commented Jul 8, 2020

bstaletic commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020

bstaletic commented Jul 8, 2020

henryiii commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020

YannickJadoul Jul 8, 2020

henryiii Jul 8, 2020

YannickJadoul Jul 8, 2020

henryiii Jul 8, 2020

YannickJadoul commented Jul 8, 2020

henryiii commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020 •

edited

Loading

bstaletic commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020

henryiii commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020

patrikhuber commented Aug 1, 2020 •

edited

Loading

patrikhuber commented Aug 1, 2020 •

edited

Loading

YannickJadoul commented Aug 1, 2020

bstaletic commented Aug 1, 2020

henryiii commented Aug 1, 2020

henryiii commented Aug 1, 2020

patrikhuber commented Aug 2, 2020

YannickJadoul commented Aug 2, 2020

		@@ -149,7 +149,7 @@ matrix:
		- os: osx
		name: Python 3.7, c++14, AppleClang 9, Debug build

Change NAMESPACE_BEGIN and NAMESPACE_END macros into PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END #2283

Change NAMESPACE_BEGIN and NAMESPACE_END macros into PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END #2283

Conversation

YannickJadoul commented Jul 8, 2020

wjakob commented Jul 8, 2020

bstaletic commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020

bstaletic commented Jul 8, 2020

henryiii commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020

YannickJadoul Jul 8, 2020

Choose a reason for hiding this comment

henryiii Jul 8, 2020

Choose a reason for hiding this comment

YannickJadoul Jul 8, 2020

Choose a reason for hiding this comment

henryiii Jul 8, 2020

Choose a reason for hiding this comment

YannickJadoul commented Jul 8, 2020

henryiii commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020 • edited Loading

bstaletic commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020

henryiii commented Jul 8, 2020

YannickJadoul commented Jul 8, 2020

patrikhuber commented Aug 1, 2020 • edited Loading

patrikhuber commented Aug 1, 2020 • edited Loading

YannickJadoul commented Aug 1, 2020

bstaletic commented Aug 1, 2020

henryiii commented Aug 1, 2020

henryiii commented Aug 1, 2020

patrikhuber commented Aug 2, 2020

YannickJadoul commented Aug 2, 2020

YannickJadoul commented Jul 8, 2020 •

edited

Loading

patrikhuber commented Aug 1, 2020 •

edited

Loading

patrikhuber commented Aug 1, 2020 •

edited

Loading