py/obj: Fix mp_obj_is_integer() recognition of bool. #5557
Conversation
|
See #5538 for discussion, and tests. |
| @@ -721,7 +721,8 @@ bool mp_obj_is_true(mp_obj_t arg); | |||
| bool mp_obj_is_callable(mp_obj_t o_in); | |||
| bool mp_obj_equal(mp_obj_t o1, mp_obj_t o2); | |||
|
|
|||
| static inline bool mp_obj_is_integer(mp_const_obj_t o) { return mp_obj_is_int(o) || mp_obj_is_type(o, &mp_type_bool); } // returns true if o is bool, small int or long int | |||
| static inline bool mp_obj_is_bool(mp_const_obj_t o) { return (o == MP_ROM_FALSE) || (o == MP_ROM_TRUE) || mp_obj_is_type(o, &mp_type_bool); } // returns true if o is bool | |||
There was a problem hiding this comment.
This probably doesn't need the mp_obj_is_type() test at all, just the test against the 2 possible values.
But, for builds where MICROPY_OBJ_IMMEDIATE_OBJS is disabled, it might be simpler (smaller code) to just have the mp_obj_is_type() test (which will work fine).
And for object representation A this function could do just: return (uintptr_t)o & 0xf == 0xe;
There was a problem hiding this comment.
Is there a reason for choosing MP_ROM_XXX over mp_const_xxx ?
There was a problem hiding this comment.
not particularly no, I fairly naively picked them from the commit that broke my code (part of the openmv codebase that uses mp_obj_is_integer()).
I didn't fully inspect the changes in the commit to properly understand what was going one, this was kind of a commit and run once I tested the end use functionality as working. I hadn't seen the other issue tracking this at this stage.
|
This looks okay. I agree with your comment on And like I said in #5338, I have an idea about how to change |
| @@ -812,6 +813,7 @@ size_t mp_obj_dict_len(mp_obj_t self_in); | |||
| mp_obj_t mp_obj_dict_get(mp_obj_t self_in, mp_obj_t index); | |||
| mp_obj_t mp_obj_dict_store(mp_obj_t self_in, mp_obj_t key, mp_obj_t value); | |||
| mp_obj_t mp_obj_dict_delete(mp_obj_t self_in, mp_obj_t key); | |||
| mp_obj_t mp_obj_dict_copy(mp_obj_t self_in); | |||
There was a problem hiding this comment.
Leftover from another commit
There was a problem hiding this comment.
ouch... that possibly means it didn't make it into my other pr if it was left uncommitted in this file :-(
|
Should work for me, I have to review the type conversion code in which I used this anyway so as long as there's a working detection it's ok. I would extend the comment on mp_obj_is_type though to clarify care should be taken when using it, listing all cases for which it might not work. And add the unit/coverage tests shown in corresponding issue. Maybe even tests proving all mp_obj_is_xxx work as expected. |
I'm curious how your code handles bool explicitly. Naively I would have thought that using |
|
To retrieve values I'm indeed using those most of the time, but for legacy reasons we also have a C++ collection of 'parameters' which are essentially key-value pairs where the value is a templated object. So originally we'd use this in C++ only and would do e.g. |
|
And the extra points go to @Jongy for actually adding unit tests to cover the fix, nice! |
|
Fixed by d9433d3 |
Fix safe mode
The function
mp_obj_is_integer()implementation// returns true if o is bool, small int or long intThe optimisation of True & False in d96cfd1 breaks the detection of bool however.
This PR restores the existing functionality by also checking for the new
MP_ROM_FALSEandMP_ROM_TRUE.