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

fix narrowing warning for param definitions #1374

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

jpreiss
Copy link
Contributor

@jpreiss jpreiss commented May 4, 2024

When compiling the Python bindings with Clang 11, the param definitions give many noisy warnings of the form:

src/modules/src/controller/attitude_pid_controller.c:328:23: warning: implicit conversion from
      'int' to 'uint8_t' (aka 'unsigned char') changes value from 262 to 6
      [-Wconstant-conversion]
PARAM_ADD(PARAM_FLOAT | PARAM_PERSISTENT, roll_kp, &pidRoll.kp)
~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/modules/interface/param.h:118:20: note: expanded from macro 'PARAM_ADD'
    PARAM_ADD_FULL(TYPE, NAME, ADDRESS, 0, 0)
    ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
src/modules/interface/param.h:110:35: note: expanded from macro 'PARAM_ADD_FULL'
    { .type = ((TYPE) <= 0xFF) ? (TYPE) : (((TYPE) | PARAM_EXTENDED) & 0xFF), \
    ~                             ^~~~

It appears to be a limitation of Clang: It is worried about storing a value greater than 0xFF in a uint8_t field .type, but it does not recognize that the ternary expression branch will never be hit because of the (TYPE) <= 0xFF check.

It can be fixed by storing TYPE & 0xFF instead. This is a no-op, but since everything's a compile-time constant it shouldn't cause any change in the generated code.

I guess few people are compiling the bindings with Clang, so I understand if you reject the PR because it adds complexity for a niche case.

@knmcguire knmcguire requested a review from ataffanel May 8, 2024 09:22
@knmcguire
Copy link
Contributor

I've assigned @ataffanel for this but he is away at ICRA for the next few weeks so I'll raise it again once he is back

@jpreiss
Copy link
Contributor Author

jpreiss commented May 17, 2024

Have fun at ICRA!

@ataffanel
Copy link
Member

Avoiding warnings is always good! Since this is functionally equivalent I can merge it.

@ataffanel ataffanel merged commit 46c1387 into bitcraze:master Jun 3, 2024
24 checks passed
@ataffanel
Copy link
Member

Thanks for the PR!

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.

3 participants