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

Support class-specific operator new and delete #755

Merged
merged 1 commit into from
Mar 22, 2017

Conversation

dean0x7d
Copy link
Member

Fixes #754.

void_t was needed to work around a GCC 4.8 issue, but it's pretty useful to have anyway.

@jagerman
Copy link
Member

Looks good to me.

@dean0x7d
Copy link
Member Author

I'd recommend this waits for v2.2 since it does affect a pretty key part of class support. Also might need to see about future-proofing for C++17 alignment-aware new.

@jagerman
Copy link
Member

C++17 support can definitely wait for 2.2, but I think the current PR fixes an important issue that is likely going to come up again in 2.1's stable lifetime (given the improved Eigen support in 2.1).

@wjakob wjakob merged commit 0d765f4 into pybind:master Mar 22, 2017
@wjakob
Copy link
Member

wjakob commented Mar 22, 2017

I've merged this now so that it can be part of 2.1

@dean0x7d
Copy link
Member Author

Note: Clang 4.0 doesn't compile in C++17mode due to #ifdef __cpp_lib_void_t. I'll push a minor fix shortly.

@jagerman
Copy link
Member

Whoops, didn't notice that.

Should we promote the gcc7/clang4 builds to non-optional? (clang 4.0 is building with the stable released version now; gcc7 is still a snapshot, so perhaps just clang 4?)

@dean0x7d
Copy link
Member Author

I just pushed a fix for clang 4.0: f0e58a6

@wjakob
Copy link
Member

wjakob commented Mar 22, 2017

Just out of curiosity: what's the issue with __cpp_lib_void_t? Does Clang define it but then not offer void_t?

@jagerman
Copy link
Member

No, it isn't working as its supposed to. http://bugs.llvm.org/show_bug.cgi?id=26086 is the issue, I believe.

@jagerman
Copy link
Member

... which basically means LLVM libc++'s void_t is broken, since doing exactly this is the entire point of std::void_t in the first place.

@dean0x7d
Copy link
Member Author

Exactly that. The new-style implementation of void_t is template<class...> using void_t = void;, i.e. no need for a helper struct. The new libc++ implements it that way, but clang 4.0 doesn't implement CWG1558 so it just ignores unused alias template arguments. The interesting thing is that this did work before, sometime in clang 3.x.

Anyway, I just removed __cpp_lib_void_t rather than adding extra compiler version checks since both gcc and clang are in c++1z mode for now.

@dean0x7d dean0x7d deleted the op_new branch March 22, 2017 21:33
@rwgk rwgk mentioned this pull request Feb 9, 2023
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.

None yet

3 participants