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

refactor: 🥅 Change all error handling to use errno, add int32_t return codes #25

Merged
merged 15 commits into from
Feb 3, 2025

Conversation

ion098
Copy link
Collaborator

@ion098 ion098 commented Jan 2, 2025

Download the template for this pull request:

Note

This is auto generated from Add Template to Pull Request

curl -o [email protected]+f58416.zip https://nightly.link/LemLib/Gamepad/actions/artifacts/2524258094.zip;
pros c fetch [email protected]+f58416.zip;
pros c apply [email protected]+f58416;
rm [email protected]+f58416.zip;

src/gamepad/screens/alertScreen.cpp Outdated Show resolved Hide resolved
src/gamepad/screens/defaultScreen.cpp Show resolved Hide resolved
@ion098 ion098 requested a review from SizzinSeal January 4, 2025 22:37
Copy link
Member

@SizzinSeal SizzinSeal left a comment

Choose a reason for hiding this comment

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

Functions should return 0 if they succeeded, not if they failed. This is standard practice, which you'll see in use in projects such as the linux kernel, and LemLib/hardware

src/gamepad/screens/defaultScreen.cpp Show resolved Hide resolved
@ion098 ion098 requested a review from SizzinSeal January 8, 2025 21:33
Copy link
Member

@SizzinSeal SizzinSeal left a comment

Choose a reason for hiding this comment

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

  • int should be used instead of uint32.
    PROS, and the other lemlib projects, return the maximum value of a 32-bit signed integer on failure, so this should do the same.

  • possible errno values and their meanings should be documented in the function doxygen documentation. See LemLib/hardware documentation

@ion098 ion098 requested a review from SizzinSeal January 22, 2025 18:45
Copy link
Member

@SizzinSeal SizzinSeal left a comment

Choose a reason for hiding this comment

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

Fix the function return values and it should be good to merge

@@ -84,7 +84,7 @@ class Button {
* gamepad::master.Up.onPress("upPress1", []() { std::cout << "I was pressed!" << std::endl; });
* @endcode
*/
bool onPress(std::string listenerName, std::function<void(void)> func) const;
uint32_t onPress(std::string listenerName, std::function<void(void)> func) const;
Copy link
Member

Choose a reason for hiding this comment

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

the functions return INT_MAX when errno is set, which is the maximum value of an int32_t. Consider changing them to int32_t for clarity, but this is not a requirement

Suggested change
uint32_t onPress(std::string listenerName, std::function<void(void)> func) const;
int32_t onPress(std::string listenerName, std::function<void(void)> func) const;

std::lock_guard lock(m_mutex);
if (std::find(m_keys.begin(), m_keys.end(), key) != m_keys.end()) return false;
if (std::find(m_keys.begin(), m_keys.end(), key) != m_keys.end()) return UINT32_MAX;
Copy link
Member

Choose a reason for hiding this comment

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

Functions should return INT32_MAX when errno is set, so it does not differ from PROS and other LemLib projects

Suggested change
if (std::find(m_keys.begin(), m_keys.end(), key) != m_keys.end()) return UINT32_MAX;
if (std::find(m_keys.begin(), m_keys.end(), key) != m_keys.end()) return INT32_MAX;

@ion098 ion098 changed the title refactor: 🥅 Change error handling in screen code to use errno refactor: 🥅 Change all error handling to use errno, add int32_t return codes Jan 30, 2025
@ion098 ion098 requested a review from SizzinSeal January 30, 2025 17:21
Copy link
Member

@SizzinSeal SizzinSeal left a comment

Choose a reason for hiding this comment

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

LGTM, although i'd like for there to be more example code in the documentation

@ion098 ion098 merged commit 34d2d8e into main Feb 3, 2025
2 of 3 checks passed
@SizzinSeal SizzinSeal deleted the refactor/fix-error-handling branch February 3, 2025 21:45
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.

2 participants