-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
IMUGyroscope: Remove unused parameter and extract branching behaviour #13020
base: master
Are you sure you want to change the base?
Conversation
This PR would probably be easier to understand if it and the commit were titled something like "IMUGyroscope: Remove unused parameter and extract branching behaviour", as otherwise it sounds like something that affects the entirety of the codebase. (In particular, note that the PR title is shown on https://dolphin-emu.org/download/ for dev builds.) |
@@ -23,7 +23,7 @@ class IMUGyroscope : public ControlGroup | |||
|
|||
StateData GetRawState() const; | |||
// Also updates the state by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment still correct after this change? Since update
was apparently never set (so it was always true
) this should probably be something like "Also updates the state" (whatever this means.)
// Set calibration to zero. | ||
m_calibration = {}; | ||
RestartCalibration(); | ||
return std::nullopt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should not return a std::optional
just to always return std::nullopt
. If IMUGyroscope::GetState
needs to return std::nullopt
after calling this function, then do so there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonus tip: If doing so in the ternary you've changed the code to is absolutely necessary, you could still pull it off with the comma operator. I don't think the ternary operator is mission-critical in this case, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never used the comma operator before. Could you show how you want me to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Against my better judgement, I will show you.
return AreInputsBound() ? GetStateInternal() : (Reset(), std::nullopt);
However, I don't recommend writing code like that. The following is what I would write instead:
if (AreInputsBound())
return GetStateInternal();
Reset();
return std::nullopt;
It may seem attractive to use the one-liner with the fancy language feature, but it's less readable when it uses such obscurities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may seem attractive to use the one-liner with the fancy language feature
It is the most attractive thing since magnets, but since the goal is readability, I'll go with the verbose option. :)
@@ -23,7 +23,7 @@ class IMUGyroscope : public ControlGroup | |||
|
|||
StateData GetRawState() const; | |||
// Also updates the state by default | |||
std::optional<StateData> GetState(bool update = true); | |||
std::optional<StateData> GetState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, theoretically I still have a PR with many fixes and improvements that would make use of this, and ideally I'll finish it one day, but it's hard to guarantee.
Tbh I don't see the point in removing this flag, it's irrelevant for performance and doesn't really hurt. It feels like this is changing the code for the sake of it (at least for this specific change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I don't see the point in removing this flag, it's irrelevant for performance and doesn't really hurt.
I like code which justifies its existence, either for performance, feature or readability reasons. Not being harmful enough to remove is not a good reason to me.
It feels like this is changing the code for the sake of it
The reason for changing this is for readability. See Martin Fowler's article about flag arguments for more details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to make it an enum for sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only use enums when there are more than two code paths.
What values should the enum contain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None, ForceUpdate? Something like that? Thanks.
bd52e9b
to
a05798b
Compare
Draft goals:
update
was right or if it was kept around for a good reason.