Skip to content

Enable unused parameter warning. Fix warnings.#1501

Merged
Riksu9000 merged 1 commit intoInfiniTimeOrg:developfrom
Riksu9000:warn-unused-parameter
Jan 24, 2023
Merged

Enable unused parameter warning. Fix warnings.#1501
Riksu9000 merged 1 commit intoInfiniTimeOrg:developfrom
Riksu9000:warn-unused-parameter

Conversation

@Riksu9000
Copy link
Contributor

Unused parameters make the code confusing and intimidating. They can also cause bugs, like in the case of MotionService. MotionService::OnNewMotionValues(x, y, z) is called before updating the values inside MotionController, but these parameters are ignored, instead using the values from MotionController, which haven't been updated yet.

DebugPins was removed, since it's not used anywhere and was causing a warning.

Include directories from other projects were marked SYSTEM, to suppress their warnings.

@Riksu9000 Riksu9000 requested a review from a team December 30, 2022 21:41


set(COMMON_FLAGS -MP -MD -mthumb -mabi=aapcs -ftree-vrp -ffunction-sections -fdata-sections -fno-strict-aliasing -fno-builtin -fshort-enums -mcpu=cortex-m4 -mfloat-abi=hard -mfpu=fpv4-sp-d16 -fstack-usage -fno-exceptions -fno-non-call-exceptions)
set(WARNING_FLAGS -Wall -Wextra -Warray-bounds=2 -Wformat=2 -Wformat-overflow=2 -Wformat-truncation=2 -Wformat-nonliteral -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unknown-pragmas -Wno-expansion-to-defined -Wreturn-type -Werror=return-type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? Is there a problem with enabling the warning?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are places where it generated a lot of noise, e.g. in NimBLE bindings, does it not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it did, which is why I added the SYSTEM option when including directories from external projects, which quiets warnings that don't come from our code.

Copy link
Collaborator

@JF002 JF002 left a comment

Choose a reason for hiding this comment

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

DebugPins was removed, since it's not used anywhere and was causing a warning.

Right! I used them when I was mostly working on a devboard, but I don't use this class anymore, and I doubt anyone does.

They can also cause bugs, like in the case of MotionService.

Oof! Good catch!

I think this is a good idea to explictly ignore parameters like you did in this PR, and the other code cleanings are more than welcome, thanks!

@Riksu9000 Riksu9000 added the maintenance Background work label Jan 10, 2023
Fix warnings.
Some clang-formatting was necessary.
DebugPins is unused and was removed.
@Riksu9000 Riksu9000 merged commit a3e14c0 into InfiniTimeOrg:develop Jan 24, 2023
@Riksu9000 Riksu9000 deleted the warn-unused-parameter branch January 24, 2023 08:43
@Riksu9000 Riksu9000 added this to the 1.12.0 milestone Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Background work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants