Deprecate object::borrowed/object::stolen - #771
wjakob merged 1 commit into
Conversation
Sorry, something went wrong.
|
(Awaiting confirmation from #770 that this actually fixes it). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this 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).
Sorry, something went wrong.
|
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)). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
The constexpr static instances can cause linking failures if the compiler doesn't optimize away the reference, as reported in pybind#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 pybind#771 in favour of just deprecating (and eventually dropping) the constexpr statics. Fixes pybind#770.
|
Okay; updated the PR to deprecate borrowed/stolen and replace all internal use with borrowed_t{}/stolen_t{}. |
Sorry, something went wrong.
|
Looks good to me. |
Sorry, something went wrong.
|
Looks good -- merged! |
Sorry, something went wrong.
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
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.