Add support for keyword-only arguments

[oremanj]

I know this was in the "not going to support it" list, but I figured out how to do it with extremely minimal overhead, so I'm politely asking you to reconsider that position. :-) All ordering/etc checks happen at compile-time, the size of func_data doesn't change, and nb_func.o gets very slightly smaller on my laptop. The 'simple' function dispatcher is completely unaffected and the changes to the 'complex' one are very minimal.

[oremanj]

I'm pretty sure the pypy failure is not my fault, as I see the same failure in #442.

[wjakob]

You are correct that this is a feature that isn't banned in any way. I just thought that the overload dispatch loop in pybind11 was way overkill. Here is some first feedback.

This actually looks really good -- remarkably simple as you say. I do have a few comments and questions though.

[wjakob]

I am curious -- is the addition of static for these constexpr parameters just general best practice, or do you think it has an actual impact here?

[oremanj]

This is mostly a habit I have because of past experience with compilers thinking constexpr non-statics need to be captured in lambdas. If I remove the lambda then these definitely aren't necessary. I was confused why some of them were previously static and some not, though.

[wjakob]

Not a fan of this one, it seems kind of complicated. I am suspicious of what a compiler like MSVC will make out of this (I've seen it struggle with things as basic as inlining a 1-line lambda function, and it is generally bad at constexpr computation).

[oremanj]

Switched to a version that doesn't use a lambda.

[wjakob]

Just to clarify: this includes nb::arg() (unnamed) and nb::arg("name")` (named) arguments?

[oremanj]

That's correct, as long as they appear before the kw_only annotation or variadic *args. I expanded on the comment a bit.

[oremanj]

Thank you for the prompt review! I made some updates in response to your comments, mostly better comments with a couple functional fixes (detecting double kw_only annotations, handling kw_only before **kwargs with no *args). Please take another look when you have a moment.

[wjakob]

Perfect! Only a few minor comments at this point.

[wjakob]

stupid question: what's the difference between keeping this code here as constexpr size_t or just making a size_t variable and then potentially overwriting it in the constexpr block? If efficiency (avoiding the template expansion) is the reason, then it would seem to me that the compiler must in any case be smart enough to not expand templates in a if constexpr (false) block to avoid potential exponential instantiation of templates in recursive constexpr programs. Anyways, this way of writing things down seems somehow more natural (closer to the behavior of a regular non-constexpr program).

[oremanj]

We need the nargs_before_kw_only value available as a constexpr in order to static_assert about it in the explicit_kw_only block, and we also need it down at the bottom of the function to initialize nargs_pos. The latter doesn't need to be constexpr, so I guess we could do something like

size_t nargs_before_kw_only = nargs;
if constexpr (explicit_kw_only) {
    constexpr size_t cx_nargs_before_kw_only = is_method_det + count_args_before_index<...>(...);
    // static_assert about cx_nargs_before_kw_only
    nargs_before_kw_only = cx_nargs_before_kw_only;
}

but that seemed like way too much trouble to go to when there's not any reason to believe the instantiation of count_args_before_index is particularly expensive. I was only trying to avoid it on principle, but the cure seemed worse than the disease in the end.

[wjakob]

Got it. Let's leave it like this then.

[wjakob]

I think this ready to go in. Could you add a changelog entry and then squash the commits?

[oremanj]

Should be all set!

[wjakob]

Thank you!