Skip to content

always_construct_holder feature to support intrusively reference-counted types #561

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 2 commits into from
Dec 15, 2016

Conversation

wjakob
Copy link
Member

@wjakob wjakob commented Dec 15, 2016

This PR addresses a regression in one of my projects following #533. When using smart pointers with intrusive reference counting, we always want to construct the holder type because it's always possible to do so (the instance is really its own holder type, that's the point of intrusive refcounts).

@wjakob
Copy link
Member Author

wjakob commented Dec 15, 2016

Hmm.. there is something really weird going on with Travis. Builds with "GCC 6.2.1" (which isn't even an existing version) fail mysteriously on super-basic stuff. I suspect a codegen issue. The same thing is also happening on the PyPy branch. I have not been able to reproduce the issue with GCC 6.2.0 on Linux, which is the latest 6.x release.

Can we change travis to use 6.2.0?

@wjakob
Copy link
Member Author

wjakob commented Dec 15, 2016

^ @dean0x7d

I suspected that ubuntu-toolchain-r-test is to blame, but this is a package from debian-testing.

@jagerman
Copy link
Member

6.2.1 is essentially upstream gcc's 6.2 stable branch. I'll investigate and see what's going on.

@jagerman
Copy link
Member

Aha, it's not a gcc issue: python 2.7.13 RC1 just hit debian testing, and it's what causes the failure.

@jagerman
Copy link
Member

(And just to confirm: building under clang 3.9/python 2.7.13 RC1 gives the same failures).

@jagerman
Copy link
Member

PR #562 will "fix" it, temporarily, by allowing that test to fail until a fixed python 2.7 propagates to debian testing. (We could fix it by using something like pytest.mark.xfail(sys.version_info = (2, 7, 13, 'candidate', 1), "broken 2.7.13 RC1"), but the one-line fix to allow failures seems simpler.)

These Python RC's are not playing nicely with pybind11.

@dean0x7d
Copy link
Member

Interesting day for Python RC builds.

Regarding the PR itself, it would be good to add a test (the intrusive pointer's refcount) to avoid regressions.

@wjakob wjakob force-pushed the intrusive-refcounting branch from c469deb to f786790 Compare December 15, 2016 22:00
@wjakob
Copy link
Member Author

wjakob commented Dec 15, 2016

Thank you @jagerman. I've become so distrustful of compilers after playing with the AVX512 vectorization backend of Clang and GCC that I didn't realize that Python could be at fault. ;)

I'll merge this then (with an added testcase)

@wjakob wjakob merged commit 2029171 into pybind:master Dec 15, 2016
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