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

set_axis_position() potential unused #27293

Merged

Conversation

classicrocker883
Copy link
Contributor

Description

in module/stepper.cpp
Stepper::set_axis_position() true intentions may be underutilized. it appears to be used only once in planner.cpp for only E_AXIS

So, I removed the call for other Axis' and renamed to set_e_axis_position to indicate its actual purpose.

I suppose this may get rid of the functions "future proofing", however, it is beyond me if this function has another use based on the original purpose.

For now, this change suites it for now.

Requirements

Benefits

Remove unused code.
Better understanding of code.

Configurations

Related Issues

@thisiskeithb
Copy link
Member

I suppose this may get rid of the functions "future proofing", however, it is beyond me if this function has another use based on the original purpose.

So why are you submitting a PR? This also doesn't pass CI checks / breaks builds without an extruder.

@thinkyhead
Copy link
Member

It makes sense to preserve set_axis_position as a standard method of this class, and for the benefit of forks that may be using it for other things. If E happens to be the only axis that uses it in our fork, the single parameter may be optimized away by the compiler by looking for the invariance. We can help it along by noticing the invariance at compile time and calling set_e_position instead of using the standard method for any axis.

@thinkyhead thinkyhead merged commit 94ce0d2 into MarlinFirmware:bugfix-2.1.x Jul 22, 2024
63 checks passed
@classicrocker883 classicrocker883 deleted the bugfix-2.1.x-July5 branch August 2, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants