Factoring out make_constructor from type_caster_base (for reuse under PR #2672).

[rwgk]

Pure, very small-scale refactoring of internals, to support reuse in new developments.

This PR enables removal of duplicate code here: df13603

In theory I could inherit from the existing type_caster_base, although I only need the make_constructor functionality, but that would muddy the waters considerably, i.e. it would be much less clear to readers of the new code what's actually needed.

[YannickJadoul]

Uuuu, multiple inheritance. I guess it should be fine (there's nothing virtual about these casters, right?), but there's really no other way to get this done? :-/

Nitpick: Let's name it make_constructor_base or make_constructor_mixin, maybe, to make clear it's a class, rather than a function. (I guess that's obvious when used as base type, but still, make_constructor sounds like a function to me.)

[YannickJadoul]

Uuuu, multiple inheritance. I guess it should be fine (there's nothing virtual about these casters, right?), but there's really no other way to get this done? :-/

Within pybind11, i see no reason to actually have this be a static member function? So just adding these as inline functions to detail might work as well and avoid MI.

But we might be breaking the semi-public interface of type_caster_base (since it's only protected).

[rwgk]

Yes, non-virtual, so multiple inheritance is straightforward/safe.
But let me try something else, a little less conservative ... hang on.

[YannickJadoul]

Yes, maybe this is the most interface-preserving change you can make, if we assume users subclassing type_caster_base would depend on this protected member.

And duplicating

template <typename T>
static auto make_copy_constructor(const T *x) -> decltype( detail::make_copy_constructor(x)) {
    return detail::make_copy_constructor(x);
}

just to avoid a mostly-safe type of MI is also quite silly.

[YannickJadoul]

Curious to see what you still have in mind, though!
Otherwise, I'm fine with this, except for maybe the suggested name change, if you agree on that.

[YannickJadoul]

By the way, you use case for refactoring this can't just subclass type_caster_base<Foo> and reuse this code, that way? (Just curious, but I understand casters are messy and completely entangled, so ... :-) )

[rwgk]

By the way, you use case for refactoring this can't just subclass type_caster_base<Foo> and reuse this code, that way? (Just curious, but I understand casters are messy and completely entangled, so ... :-) )

Did you see my "muddying the waters" comment in the PR description?

[YannickJadoul]

Did you see my "muddying the waters" comment in the PR description?

Argh, sorry! No, I seem to have skipped over that sentence and went to the code... Then yeah, makes sense ;-)

[rwgk]

Please take another look @YannickJadoul.

I'm now explicitly calling make_constructor::make_copy_constructor (and similar for move). As you pointed out already, this changes the API of type_caster_base, BUT

• All unit tests pass (locally tested only at the moment, I hope the CI is also still universally happy).
• Using our internal code search (which searches a lot of code) I found no uses of make_copy_constructor or make_move_constructor outside cast.h, except one pybind11 v2.1 unit test.
• The API change is in the detail namespace.

I think the current version is a better change, cleaner, more explicit. The only thing I'd still do, if you agree, I'd choose constructor_factory as the name for the factored-out class, so it'd look like this:

constructor_factory::make_copy_constructor

What do you think?

(While I wrote this one CI job failed, but it looks like an unrelated flake.)

[YannickJadoul]

I think this looks slightly less scary (even though it shouldn't), indeed :-) If you like this better, I'd go for this.

I still have two suggestions:

• constructor_factory::make_copy_constructor: I don't think we still have a reason to make this a static member function of a class (except for scoping, maybe, but it's in detail?). What about just making these normal inline functions in detail?
  (Only Constructor might need to be renamed to caster_constructor or something, since Constructor is a reasonably generic name?)
• What about adding these two functions still to type_caster_base (but maybe as deprecated, if you prefer):

  template <typename T>
  static auto make_copy_constructor(const T *x) -> decltype(detail::make_copy_constructor(x)) {
      return detail::make_copy_constructor(x);
  }
  template <typename T>
  static auto make_move_constructor(const T *x) -> decltype(detail::make_move_constructor(x)) {
      return detail::make_move_constructor(x);
  }
  

  I know it seems silly, but there's this grey zone of "casters are kind of public, even though they're in detail"? And it's a reasonably easy fix (and one you won't need when using the new API ;-) )

Apart from those two suggestions (they make sense to me, but if you have a good argument, I don't think they're crucial), yeah, looks good to me :-)

[YannickJadoul]

(While I wrote this one CI job failed, but it looks like an unrelated flake.)

Just got an email that CI is also failing on the same job on my #2801. So let's agree we're not to blame for that one ;-)

[rwgk]

• I don't think we still have a reason to make this a static member function of a class (except for scoping, maybe, but it's in detail?).

Hm ... I think it's much clearer to group the functions needed in an obvious way. Using a struct as the tool makes it really obvious that the functions are related. If we didn't need the ... versions for SFINAE I'd go with your suggestion, but if those functions just sit there at the namespace level, I think it reduces the quality of code organization.

Another idea:

struct make_copy_constructor {
    using Constructor = void *(*)(const void *);

    static auto make(const T *x) -> decltype(new T(*x), Constructor{}) {
        return [](const void *arg) -> void * {                                  
            return new T(*reinterpret_cast<const T *>(arg));                    
        };                                                                      
    }                                                                           

    static Constructor make(...) { return nullptr; }
};

And similar for move. (That approach nicely generalizes, although currently we just need copy and move.)

What about adding these two functions still to type_caster_base (but maybe as deprecated, if you prefer)

If I had the slightest evidence that anyone needs this, I'd say yes, but there really is none (our code search covers a lot of external code, too). Cleaner code seems the better option in this situation.

[YannickJadoul]

Hm ... I think it's much clearer to group the functions needed in an obvious way. Using a struct as the tool makes it really obvious that the functions are related. If we didn't need the ... versions for SFINAE I'd go with your suggestion, but if those functions just sit there at the namespace level, I think it reduces the quality of code organization.

But that's what namespaces are for, no? I just have a feeling we wouldn't even have thought of grouping them in a class rather than just in detail, if the old state of the code wasn't there. So in that respect, having a class or a struct seems to unnecessarily suggest you need to construct something.

I'm not sure I see the argument about (...). That's just part of the overload resolution, no? I don't see how namespaces change that.

But yeah, if you insist on keeping it this way, it's not wrong either. I wouldn't split things up, anymore, though.
And the constructor_factory name doesn't work anymore, because you don't need a factory object to create anything, so it's not a factory anymore.

If I had the slightest evidence that anyone needs this, I'd say yes, but there really is none (our code search covers a lot of external code, too). Cleaner code seems the better option in this situation.

You know that that argument doesn't fully convince me, right? There might be other code lying around that does something similar to your internal project, so I'm hesitant to break this interface, especially in a patch release 2.6.2. But arguably 2.7 could work, then?

[YannickJadoul]

OK, I found the reason that these functions are static and protected members: 2bc946b

So before this change, there were no make_..._constructor functions. I think this might be an for dropping the class scope (since it's a historical accident), but also for breaking the API (this change shows it wasn't really meant as interface). I'd still say to merge this as soon as 2.6.2 is out and only release it in 2.7, then? You're on master anyway?

[rwgk]

I'd still say to merge this as soon as 2.6.2 is out and only release it in 2.7, then?

Thanks @yannick, but that doesn't help me organically developing the new smart_holder code. I started with a very small, super conservative step in the right direction that would let me continue organically without duplicating code, but now we have another addition to the traffic pileup, as reflected in the growing number of PRs, compared to where we were before July 2020, even though we have N x more active contributors.

To not make the PR pile bigger for this really minor thing, and add to the significant overhead staying on top of it, I'll close this PR for now. I will instead keep track of things that I usually factor out on the go in a different way (e.g. mark with comments), for cleanup later.

[YannickJadoul]

Oh, sorry, I really didn't mean to block this! I think this could have soon gotten merged, no? I know @henryiii is finishing up 2.6.2, and I think we start merging 2.7 afterwards? (also, just a suggestion to mitigate the breakage and not add the backwards-compatible API)

But up to you.

[rwgk]

Np, it's fine. Keeping track of this small backlog of refactoring opportunities is more light-weight in source code comments, rather than branches & PRs.