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

Deprecate `object::borrowed`/`object::stolen` #771

Merged
merged 1 commit into from Mar 28, 2017

Conversation

Projects
None yet
3 participants
@jagerman
Copy link
Member

jagerman commented Mar 28, 2017

Provides (via a dummy template parameter hack) inline definitions of object::borrowed/object::stolen.

The constexpr static instances can cause linking failures if the compiler doesn't optimize away the reference, as reported in #770.

There's no particularly nice way of fixing this in C++11/14: we can't inline definitions to match the declaration aren't permitted for non-templated static variables (C++17 does allows "inline" on variables, but that obviously doesn't help us.)

One solution that could work around it is to add an extra inherited subclass to object's hierarchy, but that's a bit of a messy solution and was decided against in #771 in favour of just deprecating (and eventually dropping) the constexpr statics.

Fixes #770.

@jagerman

This comment has been minimized.

Copy link
Member Author

jagerman commented Mar 28, 2017

(Awaiting confirmation from #770 that this actually fixes it).

@jagerman

This comment has been minimized.

Copy link
Member Author

jagerman commented Mar 28, 2017

One comment: In C++17 this could be fixed by adding just:

inline constexpr object::borrowed_t object::borrowed;
inline constexpr object::stolen_t object::stolen;

which is, of course, much nicer than the template hack, but seeing as we need the hack for 14/11 already, I don't see much advantage in using #ifdefs to provide the alternative in C++17 mode.

@dean0x7d
Copy link
Member

dean0x7d left a comment

This looks too heavyweight. I would just replace all usages of borrowed with borrowed_t{}. The borrowed definition itself might need to stay for backward compatibility (to be removed in v3.0 with other breaking changes).

@jagerman

This comment has been minimized.

Copy link
Member Author

jagerman commented Mar 28, 2017

I'm fine with that. Do you mean heavyweight in terms of added compiler complexity, or something else? (I did check that this doesn't change sizeof(object)).

@dean0x7d

This comment has been minimized.

Copy link
Member

dean0x7d commented Mar 28, 2017

I mean complex both for the compiler and for humans. The only advantage is avoiding _t{} in the syntax, but the cost is multiple + private inheritance, template tricks, and a separate C++17 version. Definitely not worth it when the goal is simple-as-can-be tag dispatch.

Deprecated borrowed/stolen for borrowed_t{}/stolen_t{}
The constexpr static instances can cause linking failures if the
compiler doesn't optimize away the reference, as reported in #770.

There's no particularly nice way of fixing this in C++11/14: we can't
inline definitions to match the declaration aren't permitted for
non-templated static variables (C++17 *does* allows "inline" on
variables, but that obviously doesn't help us.)

One solution that could work around it is to add an extra inherited
subclass to `object`'s hierarchy, but that's a bit of a messy solution
and was decided against in #771 in favour of just deprecating (and
eventually dropping) the constexpr statics.

Fixes #770.

@jagerman jagerman force-pushed the jagerman:borrowed-stolen-definition branch from c1c5586 to 4a29ba8 Mar 28, 2017

@jagerman jagerman changed the title Provide definitions of borrowed/stolen Deprecated `object::borrowed`/`object::stolen` Mar 28, 2017

@jagerman jagerman changed the title Deprecated `object::borrowed`/`object::stolen` Deprecate `object::borrowed`/`object::stolen` Mar 28, 2017

@jagerman

This comment has been minimized.

Copy link
Member Author

jagerman commented Mar 28, 2017

Okay; updated the PR to deprecate borrowed/stolen and replace all internal use with borrowed_t{}/stolen_t{}.

@dean0x7d

This comment has been minimized.

Copy link
Member

dean0x7d commented Mar 28, 2017

Looks good to me.

@wjakob

This comment has been minimized.

Copy link
Member

wjakob commented Mar 28, 2017

Looks good -- merged!

@wjakob wjakob merged commit 6db60cd into pybind:master Mar 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jagerman added a commit that referenced this pull request Apr 6, 2017

Remove object::borrowed/stolen
PR #771 deprecated them as they can cause linking failures (#770), but
the deprecation tags cause warnings on GCC 5.x through 6.2.x.  Removing
them entirely will break backwards-compatibility consequences, but the
effects should be minimal (only code that was inheriting from `object`
could get at them at all as they are protected).

Fixes #777

@dean0x7d dean0x7d modified the milestone: v2.2 Aug 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment