Implements Defaut parameters

[astrale-sharp]

Closes #381

The question here is, i don't want to modify into_signature_info because it's used in more than one place where it doesn't need to be faillible.

But copying the function just to add in the default parameters parsing is ugly code wise.

i guess FunctionDef could just contain the unparsed unfaillible info of parameters and the parsing could just be done in register_method_registration but that info will only be used there whereas FunctionDef is used all over the place, still the best option imho

[astrale-sharp]

I gave a good look at the code and it wouldn't be easy to make SignatureInfo::into_signature_info faillible, it's used a lot in context where an error is not expected

Just having SignatureInfo carry param_attrs: Vec<Vec<venial::Attribute>> and handle it later breaks the data model idea of having parsed and ready to go information

There's also the problem of removing the #[opt] attribute which is handled by parse_attributes.

[astrale-sharp]

So one way to tackle this would be to add a

• default_parameters: Vec<Option<TokenStream>> to SignatureInfo
• into_signature_info doesn't fill that field
• process_godot_fn fills that field

[astrale-sharp]

Should I ping you for these sort of things ? I don't want to be a bother @Bromeon

[Bromeon]

Thanks a lot!

---

I gave a good look at the code and it wouldn't be easy to make SignatureInfo::into_signature_info faillible, it's used a lot in context where an error is not expected

Good point. Maybe the default parameter detection shouldn't happen inside this function? That is, make another function extract_default_params(), and use the result of that alongside the into_signature_info() result -- or pass the former into the latter.

let default_params = parse_default_params(&signature);
let signature_info =
    into_signature_info(signature.clone(), class_name, gd_self_parameter.is_some());

// or:

let default_params = parse_default_params(&signature);
let signature_info =
    into_signature_info(signature.clone(), class_name, gd_self_parameter.is_some(), Some(default_params));

---

Should I ping you for these sort of things ? I don't want to be a bother @Bromeon

Not necessary, I get notifications on all comments. Either way, I'll review depending on time and not on how often someone writes an answer 😉

[Bromeon]

You could iterate forward, set a flag must_be_default to true on the first encounter, and then fail if any subsequent parameter is not a default.

And please exhaustively handle all cases, without _. The if !allowed isn't needed after a if allowed for the same condition, as it's implied. If you match on (default, must_be_default) instead of only default, you can be more explicit and the compiler will get it.

[astrale-sharp]

I'll exhaustively handle all cases, However i think it's cleaner and clearer code with the vector being reversed as you don't have to keep a variable to bail on the right span.

If it's about performance, I doubt a signature function will get long enough that it matters.

If you insist though I'll do you it your way!

[Bromeon]

I'll exhaustively handle all cases, However i think it's cleaner and clearer code with the vector being reversed as you don't have to keep a variable to bail on the right span.

You currently have allowed, how is that different?

Not needing to reverse twice is not only faster, but also simpler -- I don't see why you'd choose extra unnecessary operations here.

[TitanNano]

You should be able to just do something like:

sig_info.param_defaults
    .iter()
    .skip_while(|pd| pd.is_none())
    .cloned()
    .collect::<Option<Vec<_>>>()
    .some_or_else(...)

[astrale-sharp]

Not necessary, I get notifications on all comments. Either way, I'll review depending on time and not on how often someone writes an answer 😉

That's a relief, I have a tendency to yap too much!

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1213

[astrale-sharp]

If you want the args to implement AsArg I could add call to a dummy noop function that only accepts T: AsArg or did you have something in mind? like what's the point of this request so i can satisfy it the best?

[astrale-sharp]

Next order of business, change make_method_registration
call to make_varcall_fn so that it adds an argument (default_parameters length)

make_varcall_fn can be changed cause it's only used here

Then change in_varcall logics (CallError::check_arg_count) to check the number argument (something like args_count + default_parameters_length >= param_count)

[Bromeon]

If you want the args to implement AsArg I could add call to a dummy noop function that only accepts T: AsArg

No, you'll need the trait to convert the argument to an actual Variant value. See how Array::push() does it, for example 🙂