Skip to content

Add support for function argument defaults - #100

Merged
windelbouwman merged 4 commits into
RustPython:masterfrom
OddBloke:kwargs
Aug 29, 2018
Merged

windelbouwman merged 4 commits into
RustPython:masterfrom
OddBloke:kwargs

Conversation

@OddBloke

Copy link
Copy Markdown
Collaborator

This allows defaults to be specified at function definition:

def func(pos, defaulting="default"):
    return pos, defaulting

assert 1, 2 == func(1, 2)
assert 1, "default" == func(1)

Comment thread vm/src/bytecode.rs
}
}

bitflags! {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would go for an extra boolean in the function: has_defaults. So no extra packages.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will eventually need to carry 4 values (per https://docs.python.org/3/library/dis.html#opcode-MAKE_FUNCTION), which is why I went with the bitflags approach. It also maintains symmetry with the CPython implementation.

If you'd still prefer for me to change it, I'd be happy to do so.

(FWIW, we also already pull bitflags in due to a couple of dependencies, so this has no net impact of our dependency tree.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasons would be that using booleans would allow the rust compiler to implement bitfields by itself. I think bitfields are an optimization from times where memory was scarce and compilers did not implement this. I would not be surprised if rust under the hood implemented booleans as bitfields.

Leave it as it is for now :)

Comment thread vm/src/vm.rs Outdated
for (name, value) in code_object.arg_names.iter().zip(args.args) {
let nargs = args.args.len();
let nexpected_args = code_object.arg_names.len();
let args = match nargs {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use match in this case, but a sequence like this:

if nargs > expected {
} else if nargs < expected {
} else {
}

Comment thread vm/src/vm.rs Outdated
for (name, value) in code_object.arg_names.iter().zip(args.args) {
let nargs = args.args.len();
let nexpected_args = code_object.arg_names.len();
let args = match nargs {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use match here, but if-elseif-else:

if nargs > expected {
...
} else if nargs < expected {
...
} else {
...
}

@windelbouwman

Copy link
Copy Markdown
Contributor

Nice job!

The enforcement of the ordering of default/non-default arguments happens
in the compiling step, but should perhaps eventually be pushed back in
to the parsing phase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants