-
Notifications
You must be signed in to change notification settings - Fork 506
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
remove superfluous eval conditions #7984
base: master
Are you sure you want to change the base?
remove superfluous eval conditions #7984
Conversation
Hey @bluemoehre, thanks for your pull request! Tip Modified bundles can be downloaded here. DDB changesModified
ValidationTip Everything is fine ! 🕟 Updated for commit ffdb8f3 |
@ebaauw FYI |
While this is technically correct, there's devices out there depending on this check to discard blips. Given the fact that this check doesn't hurt and there is a necessity, we should keep this untouched. |
Do we know which ones?
That's bad practice. Like @ebaauw asked me to change this (at least for the generic one), and I found the same "error" in other files, it may happen again that people reading the code want to fix that, because it looks like a bug. If only a few people have that hidden knowledge, it's a risk. |
I'm afraid I cannot follow you to a certain extend. Where is the bad practice? We're discussing a valid boundaries check here being there for a reason. This is neither a bug (which is by definition a programatical misbehavior), nor an error (as nothing is working falsely) so there is nothing to fix in my view. Leaving the code as is has 0 risk, changing the code as proposed introduces an unneccessary risk of breaking functionality of an unknown number of devices, hence I prefer avoiding that risk. Regarding your suggesting to add a respective comment in the code, I'd fully buy in. |
It's technically not possible for an |
"Given the fact that this check doesn't hurt" [...] "we should keep this untouched" It's bad practice to keep a workaround in a generic file just because it doesn't hurt. Especially if only a few devices are to be fixed with it. The generic files should kept clean all the time, since the amount of devices will continuously keep growing. (Imagine having in there all workarounds for all types of devices - that won't work out.) Also we should not keep this untouched, since the workaround is not documented at all. Undocumented workarounds will lead to copies pasted somewhere. If you need to clean this up in the future, it will take a lot of effort.
It is not, as @ebaauw already described. 1 Bit is used for the sign, 15 Bits remaining for the magnitude: According to the specification the value always must also be an I checked all the devices that were changed by this PR and their commits to see if there was any information about special treatment required. If there really was a problem, it was unfortunately not documented. But you may check yourself:
That's all I can do at this point to convince everyone that we are doing the best possible here. |
Signed s16 integer supports values from
-32768
to32767
, soAttr.val != 32768
is superfluous. Typically the lowest value,-32768
is used to indicate an invalid value.