full kwargs handling for property()

[chrysn]

given it simplifies the handling of different numbers of arguments (deleted code portion), i'd offer this as my preferred solution to #1778, providing maximum compatibility with cpython at the cost of four additional qstr, and probably slightly increased stack size and runtime for property calls (not measured).

[dpgeorge]

Can you provide code size change for unix and stmhal ports?

A way to decrease size would be not to have qstrs for fset, fget, fdel, and instead just use MP_QSTR_. That should actually just work without any change to the argument parsing code. The only difference in running is that if you do specify, eg, fset as a kw arg then it'll give an error "extra keyword arguments given". But at least it'll be an error instead of silently doing the wrong thing.

[dpgeorge]

There is a new pattern for doing this that allows you to access the arguments by named struct field: https://github.com/micropython/micropython/blob/master/stmhal/uart.c#L460-L465

[chrysn]

unix on amd64 text size before: 349705, original patch: 349865 (+160), with empty MP_QSTR_ 349801 (+96).
stmhal against newlib before: 282592, original patch: 282684 (+92), with empty MP_QSTR_: 282664 (+72).

as for the new pattern: the struct approach is nice (although i coudn't cite a c rule that guarantees the intended behavior, although i can't think of an architecture where it wouldn't just work). the function calls and allocations, however, seem to still be the same (the mp_map_t allocation and initialization just happens outside in pyb_uart_make_new. or is it the MP_DEFINE_CONST_FUN_OBJ_KW that makes the difference?)

if you are confident that the struct instead of the array is legal c, i'll happily update the patch, likewise if you feel that the 20 to 44 bytes of leaving out fget=/fset=/fdel= are worth it.

[dpgeorge]

although i coudn't cite a c rule that guarantees the intended behavior

Doesn't C have to keep the order of members in a struct? Don't people use structs a lot to map to binary structure of a file (eg elf)?

[dpgeorge]

I actually just push a patch to use the new pattern in py/ core: 22d85ec I hope it is indeed legal C, because if not we need to understand that right now and revert this change...

You should use mp_arg_parse_all_kw_array instead of mp_arg_parse_all. That will also help to reduce code size. Can you please report sizes with that?

I'd rather go the empty qstr route for fget/fset/fdel, at least until someone runs into an issue with this and it needs to be done 100% compatible with CPython.

[dpgeorge]

Another improvement we can make is to use only 16 bits for the qstr and flags members in the mp_arg_t struct. That's ok so long as there are no more than 65536 static qstrs, which is pretty well guaranteed (can still have more than that at runtime, that'll still be ok).

[chrysn]

On Wed, Jan 13, 2016 at 08:08:55AM -0800, Damien George wrote:

Doesn't C have to keep the order of members in a struct? Don't people
use structs a lot to map to binary structure of a file (eg elf)?

the order is guaranteed, i'm just not fully sure that the struct members
are guaranteed to be aligned the same way array members are.

in practice, they are; probably they are even guaranteed to, and i'm
just overly cautious because i can't find it in the standard.

[dhylands]

The C99 spec says (6.7.2.1 para 13) says:

"Within a structure object, the non-bit-field members and the units in which bit-fields
reside have addresses that increase in the order in which they are declared. A pointer to a
structure object, suitably converted, points to its initial member (or if that member is a
bit-field, then to the unit in which it resides), and vice versa. There may be unnamed
padding within a structure object, but not at its beginning."

[dpgeorge]

There may be unnamed padding within a structure object

Going to the letter of the spec, that means we can't use the pattern. The compiler may insert padding between the struct members (not likely though).

So... maybe we can use the pattern just in stmhal, but not py/? Or switch to using enums to define named offsets within an array? Any other suggestions?

[dpgeorge]

Using the enum approach is relatively clean. For example:

enum { ARG_fget, ARG_fset, ARG_fdel, ARG_doc };
static const mp_arg_t allowed_args[] = {
    { MP_QSTR_fget, MP_ARG_OBJ, {.u_rom_obj = mp_const_none} },
    { MP_QSTR_fset, MP_ARG_OBJ, {.u_rom_obj = mp_const_none} },
    { MP_QSTR_fdel, MP_ARG_OBJ, {.u_rom_obj = mp_const_none} },
    { MP_QSTR_doc, MP_ARG_OBJ, {.u_rom_obj = mp_const_none} },
};
mp_arg_val_t vals[MP_ARRAY_SIZE(allowed_args)];
mp_arg_parse_all_kw_array(n_args, n_kw, args, MP_ARRAY_SIZE(allawed_args), allowed_args, vals);
...
vals[ARG_doc].u_obj

That looks like legal and portable C.

[chrysn]

You should use mp_arg_parse_all_kw_array instead of mp_arg_parse_all.
That will also help to reduce code size.

great, that's what i've been looking for.

Can you please report sizes with that?

latest unix is down to 349737 (+32 relative to original), stmhal to
282636 (+44 relative to original).

I'd rather go the empty qstr route for fget/fset/fdel

done

we can make is to use only 16 bits for the qstr and flags members in
the mp_arg_t struct

not that it'll happen in this pull request, but for comparison: unix
went to 349625 (112 bytes saved), stmhal to 282224 (412 bytes saved)
with no compiler warnings when setting the members to uint16_t; unix
tests passed.

On Wed, Jan 13, 2016 at 09:05:05AM -0800, Damien George wrote:

enum { ARG_fget, ARG_fset, ARG_fdel, ARG_doc };

done

That looks like legal and portable C.

i looked it up: enums are guaranteed to start at 0 and increment by 1.

in parallel to this pull request, which gets updated by the changes you
suggested, i've issuing a single-patch version #1780 (my preference for
fine-grained commits is not shared by everybody); merge and reject as
you like.

[pfalcon]

Superseded by #1780.