[multitop] Cleanup DT templates #26032
Conversation
pamaury
force-pushed
the
multitop_sw_dt_cleanup
branch
from
942e55c to
01680a1
Compare
pamaury
force-pushed
the
multitop_sw_dt_cleanup
branch
2 times, most recently
from
8eeb67b to
f685b0c
Compare
| """ | ||
| An array type that maps values to other values. | ||
| """ | ||
| def __init__(self, elem_type: BaseType, index_type: BaseType, length: object): |
There was a problem hiding this comment.
Did you mean to type length as object rather than int?
There was a problem hiding this comment.
Yes, this length can be anything, it is rendered by index_type.render_value(length)
| def add_field(self, name: Name, field_type: BaseType, docstring: str = ""): | ||
| self.fields[name] = (field_type, docstring) | ||
|
|
||
| def has_field(self, name: Name) -> bool: | ||
| return name in self.fields |
There was a problem hiding this comment.
If possible I would try and use magics to implement this kind of operations. This means you can then use the built in operators such as += or in.
https://realpython.com/python-magic-methods/#membership-operators
There was a problem hiding this comment.
I understand your suggestion. However in this case I prefer to be explicit for readable of the users.
There was a problem hiding this comment.
Makes sense looking more at how it's used. Btw, if you want to make custom types that act more like builtin types then you can use the collections.abc.* ABSs. So for example if you wanted to implement a list like object you can inherit from collections.abc.Sequence, which will then require you implement certain magics before the class will instantiate. Personally I've found this really helpful to know which magics are required to implement which interfaces.
Also it's good practice to use these interfaces in typing your methods and functions, and make them as broad as you can to match your need. Otherwise typing becomes difficult to meet as the code changes over time. So personally I would prefer a collections.abc interface over a builtin collection type, as this also allows any custom object that implements that interface. Same goes for custom classes, where Protocols can be used instead of concrete class types.
For example def f(s: list[str]) -> None: will take list only otherwise type checkers will complain. But what if your not intending to mutate the list object passed... then you can use Sequence[str] instead, and also allow tuple or any other list-like object. Whereas if you use MutableSequence then for example tuple is not valid as you can't mutate the tuple itself, but it still allows the flexibility in the typing to use a custom list like object instead of a builtin list. To take this further if you don't need to know the length of the object or get an item by index but still need to be able to check ownership within the container, then you can use Collection instead... which would allow set objects and others to be passed. If all you need to do is to be able to iterate over the items and don't care about order &c. then Iterable is all you need. Just be aware if you do that then the object passed may be a generator for example and only be iterable once. Quite often I'll use Iterable and if I need to iterate over multiple times or pass to another function that may consume the object by iterating it, then I'll convert it to a tuple at the beginning of the function. i.e. new = tuple(old).
There is a useful table here that explains the different ABCs available. Many of those magic methods you get for free by just sub-classing from the base class, as they can be derived from other magics.
https://docs.python.org/3/library/collections.abc.html#collections-abstract-base-classes
pamaury
force-pushed
the
multitop_sw_dt_cleanup
branch
3 times, most recently
from
8659d0a to
7466fb1
Compare
The API is clearly oriented toward a "frozen" Name that cannot change after it is created. However the "parts" field is public. This commit makes the "parts" field private but accessible for reading. Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Previously, there was a `size_t dt_instance_index(dt_inst_id_t id)` function to get the "index" of an instance ID. This API was unclear and not type safe. This API is replaced by a per-IP `dt_<ip>_t dt_<ip>_from_instance_id(dt_inst_id_t id)` which essentially does the same thing. For example, `dt_uart_from_instance_id(kDtInstanceIdUart3) == kDtUart3`. Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
multitop_sw_dt_cleanup
branch
from
7466fb1 to
7fe7c8f
Compare
The templates for the various files were extremely difficult to read, mixing a lot of logic with a lot of formatting. Furthermore, the same logic was duplicated between files, which could lead to consistency issues. The new code moves all the logic to python helpers that create data structures, the template only focus on formatting and rendering. Since the dt uses more complex structures than topgen, e.g. nested structures with nested array mapping, a complete set of new classes to deal with this complexity. Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
multitop_sw_dt_cleanup
branch
from
7fe7c8f to
c620c6d
Compare
Move most of the logic out of the templates and into helpers
The templates for the various files were extremely difficult to read, mixing a lot of logic with a lot of formatting. Furthermore, the same logic was duplicated between files, which could lead to consistency issues. The new code moves all the logic to python helpers that create data structures, the template only focus on formatting and rendering.
Since the dt uses more complex structures than topgen, e.g. nested structures with nested array mapping, a complete set of new classes to deal with this complexity.
This PR only makes one modification to the API:
size_t dt_instance_index(dt_inst_id_t id)function to get the "index" of an instance ID.This API was unclear and not type safe.
dt_<ip>_t dt_<ip>_from_instance_id(dt_inst_id_t id)which essentially does the same thing. For example,dt_uart_from_instance_id(kDtInstanceIdUart3) == kDtUart3.