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

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

Open
cousteaulecommandant opened this issue Jul 10, 2024 · 1 comment

Comments

@cousteaulecommandant
Copy link
Contributor

cousteaulecommandant commented Jul 10, 2024

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

  • hw/core-v-mini-mcu/peripheral_subsystem.sv.tpl
  • util/mcu_gen.py

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 cousteaulecommandant changed the title Python: <string_var> in ("<string>") should be `<string_var> == "<string>" Python: <string_var> in ("<string>") should be <string_var> == "<string>" Jul 10, 2024
@cousteaulecommandant
Copy link
Contributor Author

cousteaulecommandant commented Jul 12, 2024

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.)

cousteaulecommandant added a commit to cousteaulecommandant/x-heep that referenced this issue Jul 12, 2024
The use of `in` for string comparison results in `"name" in "name2"`
giving a false positive; the right thing is to use `==`.
In peripheral_subsystem.sv.tpl, this may result in the peripheral
being added twice, or being added when it shouldn't.
This has been fixed in this commit.

In addition, the way in which dict keys are handled has been changed
from iterating through all the keys to just finding the relevant key.
cousteaulecommandant added a commit to cousteaulecommandant/x-heep that referenced this issue Jul 12, 2024
This is the same issue that was found at bug esl-epfl#548, although in this case
it has not shown any adverse effects *for now*.
Nevertheless, it is never a bad idea to fix it as well just in case.

Also fixed file `pad_cfg.hjson` which incorrectly adds a comma after
quoteless string `active: low,` which isn't valid Hjson.
This worked before because `"low" in "low,"` is technically true,
but this was just one bug hiding another bug.
This issue affects several other files and will be reported separately.
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

No branches or pull requests

1 participant