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
Emit a diagnostic when parentheses are omitted in logical expressions with comparisons #380
Comments
Very footgun. Much debug. I'm wondering if just steering people to use a different construct as a best practice would be sufficient. For example, instead of this, which is prone to error: with m.If((self._instr_phase == 0) & ~self._execute_trap): Use: with m.If(all(self._instr_phase == 0, ~self._execute_trap)): or something similar to |
That doesn't actually work, try it. (Unless we override I know how to detect this issue mostly reliably; it just needs to be implemented. |
I mean something like this (which seems to have worked): def allcond(*args):
cond = 1
for arg in args:
cond &= arg.bool()
return cond with m.If(allcond(self._instr_phase == 0, ~self._execute_trap)): |
Could make this |
I'm not a fan. This trades off some complexity in the DSL for another kind of complexity that is not clearly better, and in addition, effectively splits the language into two dialects: one that uses |
So you'd be able to detect |
Er, nevermind, I misread what you wrote. |
E.g.
foo and bar == 0
is OK, butfoo & bar == 0
is parsed differently due to precedence, and virtually always results in a nasty bug.It's not completely clear what would be the robust pattern to detect here, but it's the single biggest nMigen footgun, so it's probably better to come up with a less reliable than ideal diagnostic than have nothing at all.
See also #159, which fixes a similar issue. The diagnostic in #159 is only triggered in
m.If
conditions, which is (now that I think about it) incomplete; it's entirely possible that someone will capture or assign such an expression and get a broken design anyway. Ideally we also need a more robust fix for that.The text was updated successfully, but these errors were encountered: