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

Minor cleanup #560

Merged
merged 4 commits into from
Aug 25, 2020
Merged

Minor cleanup #560

merged 4 commits into from
Aug 25, 2020

Conversation

henrygab
Copy link
Collaborator

@henrygab henrygab commented Aug 24, 2020

Leftovers from #496.

  • Remove semaphore from tone()
  • Remove 'inline' ... it's not related in any way to compiler optimization, but rather defines whether the definition can be within a header file, which is not applicable here (it's not __attribute__((always_inline)))

@@ -74,7 +73,7 @@ class TonePwmConfig {
};
TonePwmConfig _pwm_config;

inline static bool _is_pwm_enabled(NRF_PWM_Type const * pwm_instance) {
static bool _is_pwm_enabled(NRF_PWM_Type const * pwm_instance) {
Copy link
Member

Choose a reason for hiding this comment

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

inline is still OK, it is more like suggestion to gcc. It is up to gcc to decide e.g Ofast may inline the function, but Os may not since it increase the size. attribute always inline is more like directive to gcc to force it to do so. So yeah, it is normally not an issue on non timing-sensitive function. Anyway, I normally just put inline where I would like, and leave the decision to gcc.

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Thanks for removing the lamda function, I think I am too conservative for it. Maybe I should update my C/C++ knowledge, still writing C99 for now.

@hathach hathach merged commit 7993737 into adafruit:master Aug 25, 2020
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