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

Fix middle height computation in 3D dynamics #610

Closed
wants to merge 1 commit into from

Conversation

kvark
Copy link
Contributor

@kvark kvark commented Nov 5, 2022

The semantics (and usage) of the function is such that it returns 4 bytes: lower height, middle height, upper height, and 0xFF. I believe the brackets "(" and ")" were accidentally put in the wrong place here for the middle term.

The semantics (and usage) of the function is such that it returns 4 bytes: lower height, middle height, upper height, and 0xFF.
I believe the brackets "(" and ")" were accidentally put in the wrong place here for the middle term.
@CLAassistant
Copy link

CLAassistant commented Nov 5, 2022

CLA assistant check
All committers have signed the CLA.

@stalkerg
Copy link
Contributor

stalkerg commented Mar 23, 2024

It is used only in #ifdef MSG_OUT, which is set only if DEBUG is defined. At least we have no issue for now because nobody uses such a DEBUG, especially in dynamics.cpp

@stalkerg stalkerg closed this Mar 23, 2024
@kvark
Copy link
Contributor Author

kvark commented Mar 23, 2024

@stalkerg wait, so if it's only used in DEBUG, it's fine to keep this bug? Why not land the fix?

@stalkerg
Copy link
Contributor

@kvark do we really use DEBUG? We have no formal way to test your change.

@kvark
Copy link
Contributor Author

kvark commented Mar 30, 2024

If we don't use DEBUG and we are concerned about the relevant code being untested, we might as well remove everything that is gated on debug. The current position is inconsistent: we have the code, we know it's broken, and we are refusing a patch to it?

@stalkerg
Copy link
Contributor

this code here for historical reason, I suppose we easily can remove it.

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

Successfully merging this pull request may close these issues.

3 participants