mp_const_none and friends are lvalue variables

[pfalcon]

As I suspected, mp_const_none and friends are not constant expressions, but variables with const modifier. Besides being non-efficient, it's also make impossible to do stuff like:

static mp_obj_t arr[] = {mp_const_none};

Instead of current impl, we should define them as:

#define mp_const_none (mp_obj_t)&mp_const_none_obj

[pfalcon]

While solving this, let's also make following change:

typedef struct foo foo;

I.e. make sure that struct name matches typedef name, so they can be used interchangably (and we don't need to add typedef for each forward struct declaration which would be need to declare mp_const_none_obj and friends).

[pfalcon]

Here's example:

--- a/py/obj.h
+++ b/py/obj.h
@@ -226,13 +226,18 @@ extern const mp_obj_type_t mp_type_ValueError;
 extern const mp_obj_type_t mp_type_ZeroDivisionError;

 // Constant objects, globally accessible
-extern const mp_obj_t mp_const_none;
+//extern const mp_obj_t mp_const_none;
 extern const mp_obj_t mp_const_false;
 extern const mp_obj_t mp_const_true;
 extern const mp_obj_t mp_const_empty_tuple;
 extern const mp_obj_t mp_const_ellipsis;
 extern const mp_obj_t mp_const_GeneratorExit;

+struct _mp_obj_none_t;
+typedef struct _mp_obj_none_t mp_obj_none_t;
+extern const mp_obj_none_t mp_const_none_obj;
+#define mp_const_none ((mp_obj_t)&mp_const_none_obj)
+
 // General API for objects

 mp_obj_t mp_obj_new_type(qstr name, mp_obj_t bases_tuple, mp_obj_t locals_dict);
diff --git a/py/objnone.c b/py/objnone.c
index 1ec3d46..43f8841 100644
--- a/py/objnone.c
+++ b/py/objnone.c
@@ -29,5 +29,5 @@ const mp_obj_type_t none_type = {
     .unary_op = none_unary_op,
 };

-STATIC const mp_obj_none_t none_obj = {{&none_type}};
-const mp_obj_t mp_const_none = (mp_obj_t)&none_obj;
+const mp_obj_none_t mp_const_none_obj = {{&none_type}};
+//const mp_obj_t mp_const_none = (mp_obj_t)&mp_const_none_obj;

[dpgeorge]

Yes, their reason for existence is just for convenience, so you don't have to type (mp_obj_t)&mp_const_none everywhere. As you can see, constant exceptions and types are not like this, and you need to take their address explicitly (and add a cast).

A macro is a good solution, as it simplifies, retains ease of use, and reduces some RAM usage. May also reduce ROM usage, since &mp_const_none_obj can be filled in as a constant by the linker, wherease mp_const_none as it stands now requires a memory read.

[dpgeorge]

Done. Code size for x64 reduced by 216 bytes, stm, by 280. And uses less RAM, so a good fix all round!

Only None, False, True and empty tuple have macros, since they are the most used.

I don't understand the point about struct typedefs and naming. I didn't need any typedefs to implement this.

[pfalcon]

I don't understand the point about struct typedefs and naming. I didn't need any typedefs to implement this.

Yes, instead in obj.h there's

extern const struct _mp_obj_none_t mp_const_none_obj;

while objnone.c has:

const mp_obj_none_t mp_const_none_obj ...;

Well, that's not consistent way to define a type of an object. One way to improve consistency is to at least drop leading underscore on struct names, so mp_obj_none_t and struct mp_obj_none_t was the same type. Surely, that's pedantic matter, but if you're ready to rename rt_ to mp_, maybe it's worth to do this either.

[pfalcon]

Thanks for fixing this for all the common objects btw!

[dpgeorge]

For struct s, the name s is a tag and lives in a separate namespace to type names, so you don't need to have the structure tag the same as the typedef.

The way it is now, it still has type checking, because struct _mp_obj_none_t is equivalent to mp_obj_none_t after the typedef in objnone.c. If you make a typo in the struct tag name in obj.h, you'll get a compile error.

[pfalcon]

For struct s, the name s is a tag and lives in a separate namespace to type names, so you don't need to have the structure tag the same as the typedef.

More important is the opposite - you don't have to have different names! And calling the same thing different names (even if it's just an extra '_') is always a confusion.

The way it is now, it still has type checking, because struct _mp_obj_none_t is equivalent to mp_obj_none_t after the typedef in objnone.c.

Yeah, but the talk is not about satisfying a compiler (it's all ok on that front), but about satisfying a human reader. The stuff as it is now looks confusing for newcomer, and not very clean for someone who waded thru it. For example, my instinctive visual memory pattern matching stops on first mismatch, and treats "foo" and "_foo" as different things. In this regard, having "struct foo_t" and "foo_t" much more intuitively suggests that it's the same thing.

[dpgeorge]

I guess it's just the training that one has had: to my eyes, I always like to see an underscore after a struct :)

I would go with your suggestion, but I think maybe that it will break C++ compatibility. C++ puts struct tags in the same namespace as typedefs, so that may cause big issues having the same tag as typedef.