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.

Sign up for GitHub

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

Jump to bottom

global: Change the IF statement to a SWITCH statement #27043

Closed
wants to merge 2 commits into from
Closed

global: Change the IF statement to a SWITCH statement #27043

wants to merge 2 commits into from

Conversation

muramura
Copy link
Contributor

In this case, where the same variable value is evaluated multiple times in IF statements,
I think it would be better to use a SWITCH statement.

@muramura muramura changed the title AP_Param: Change the IF statement to a SWITCH statement global: Change the IF statement to a SWITCH statement May 12, 2024
magicrub
magicrub requested changes May 16, 2024
View reviewed changes
libraries/AP_Param/AP_Param.cpp
if (value < 0) rounding_addition = -rounding_addition;
float v = value+rounding_addition;
v = constrain_float(v, INT8_MIN, INT8_MAX);
((AP_Int8 *)this)->set(v);
break;
}
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need this default. Also, if we change var_class to be an enum class then we'll NEED to remove the default.

@peterbarker
Copy link
Contributor

Needs a rebase (Rover changes already merged), and Tom's review comment addressed.

@muramura are you still wanting to chase this?

@magicrub I had a quick play with moving from ap_var_type to enum class AP_Param::VarType. The changes are massive as we use AP_PARAM_FLOAT` and friends in huge amounts of code. The conversion tables are possibly also slightly more difficult to read. I pushed that incomplete branch here: https://github.com/peterbarker/ardupilot/pull/new/archive/pr/vartype-enum-class - but I don't think I'll try to push that forward at this point.

@peterbarker
Copy link
Contributor

No response on this one. Please create a fresh PR if you want to pursue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magicrub magicrub magicrub requested changes

Assignees
No one assigned
Labels
NeedAnswer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@muramura @peterbarker @magicrub