Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py: Support object __getattr__, __delattr__ and __setattr__ methods #2687

Closed
wants to merge 9 commits into from
Closed

Conversation

dmazzella
Copy link
Contributor

Related to #2375

STATIC const mp_rom_map_elem_t object_locals_dict_table[] = {
#if MICROPY_CPYTHON_COMPAT
{ MP_ROM_QSTR(MP_QSTR___init__), MP_ROM_PTR(&object___init___obj) },
#endif
#if MICROPY_CPYTHON_COMPAT
{ MP_ROM_QSTR(MP_QSTR___new__), MP_ROM_PTR(&object___new___obj) },
#endif
#if (MICROPY_CPYTHON_COMPAT && MICROPY_PY_ATTRS_METHODS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just #if MICROPY_PY_ATTRS_METHODS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

becouse i have placed it into:

#if MICROPY_CPYTHON_COMPAT
....
#endif

Copy link
Contributor

@stinos stinos Dec 15, 2016

Choose a reason for hiding this comment

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

Ah didin't see that in the github diff. Still it's a bit confusing as now one might use #define MICROPY_PY_ATTRS_METHODS 1 but it won't have any effect unless MICROPY_CPYTHON_COMPAT is also defined. But maybe there's no other way. Though actually this could use some cleanup: everything is already wrapped in #if MICROPY_CPYTHON_COMPAT starting at line 44 so I don't immediately see why it needs duplication.

@@ -636,6 +636,12 @@ typedef double mp_float_t;
#define MICROPY_PY_DESCRIPTORS (0)
#endif

// Whether to support object (__getattr__, __delattr__ and __setattr__)
// This costs some code size and makes all load attrs and store attrs slow
Copy link
Contributor

Choose a reason for hiding this comment

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

'slower' would be better wording imo, without measuring the actual impact it's hard to make absolute statements about it.

@dmazzella
Copy link
Contributor Author

Is correct to use: mp_obj_dict_get_map(self_in)

instead of:

mp_obj_type_t *type = mp_obj_get_type(self_in);
if (type->locals_dict != NULL) {
    mp_map_t *locals_map = &type->locals_dict->map;
   ...

??

@stinos
Copy link
Contributor

stinos commented Dec 15, 2016

Doesn't seem equivalent at first sight. Does everything still work if you use #define MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG 0? That would make the mp_check_self macro have effect so mp_obj_dict_get_map will assert unless a dict is passed in.

@dpgeorge
Copy link
Member

To enable the tests, please enable this new feature in unix/mpconfigport_coverage.h and then also add a new test file.

@dmazzella
Copy link
Contributor Author

Rebase

@dmazzella dmazzella closed this Dec 15, 2016
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.

None yet

3 participants