esl-epfl / x-heep

eXtendable Heterogeneous Energy-Efficient Platform based on RISC-V
Other
135 stars 72 forks source link

Python: `<string_var> in ("<string>")` should be `<string_var> == "<string>"` #548

Open cousteaulecommandant opened 1 month ago

cousteaulecommandant commented 1 month ago

This issue affects the following files (that I noticed):

For some reason, the Python in operator is being used to compare a string variable with a string value, rather than using == directly.

These are correct:

s in ("foo", "bar", "baz")
s in ("foo", )
s in ["foo"]
s == "foo"

These are not:

s in   "foo"
s in  ("foo")
s in (("foo"))

Specifically, "foo" in ("foolish") will yield True since "foo" is a substring of "foolish". (Note that simply adding parentheses to something won't create a sequence, but adding brackets or a trailing comma will.)

As a result, adding two peripherals with a similar name (e.g. "gpio" and "gpio_improved") can result in one of the peripherals being included twice, or result in the peripheral being included even when it is disabled in mcu_cfg.hjson.

I imagine the use of in was justified for comparing a string against multiple possible values, but for single values it should be replaced with ==. (Or, if there really is a reason to use in instead of ==, the right-hand side MUST be a list/tuple, e.g. ("foo",) with a , at the end.)

cousteaulecommandant commented 1 month ago

Also, I don't understand the reason for iterating on peripherals.items(). It would make more sense to do either

% if peripherals['gpio']['is_included'] == 'yes':

or

% if 'gpio' in peripherals:
% if peripherals['gpio']['is_included'] == 'yes':

or

% if 'gpio' in peripherals and peripherals['gpio']['is_included'] == 'yes':

depending on whether or not the 'gpio' key is ensured to exist and what you want to do when it doesn't, and remove the % for peripheral in peripherals.items(): line.

(My understanding is that all keys are expected to exist, because it they don't then the % else: part tying the signals to zero will never be added.)