Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix some linting errors noted by clang-tidy #999

Merged
merged 1 commit into from
Sep 13, 2024
Merged

fix some linting errors noted by clang-tidy #999

merged 1 commit into from
Sep 13, 2024

Conversation

2bndy5
Copy link
Member

@2bndy5 2bndy5 commented Sep 8, 2024

I ran this lib through clang-tidy (with a basic/non-restrictive config -- see added .clang-tidy file) and found some linting errors...

Changes

  • enable generation of compilation database for all CMake builds
  • add .clang-tidy config file
  • resolve implicit casting cases
  • resolve else after return cases
  • ignore false-positive about "memory allocated with zero size"

No CI checks

Clang-tidy heavily depends on having a compilation database to understand how the sources are compiled. The compilation database is basically a JSON file that stores all compiler commands for each file.

Why not?

A compilation database is specific to the compilation target. To check the Pico SDK examples we'd need to run clang-tidy with different parameters (the -p=build option).

Currently, we're using my side-project called cpp-linter to run clang-format and get PR suggestions. While it could also run clang-tidy and provide similar code suggestions in a PR, cpp-linter would have to be run multiple times for each compilation target. Unfortunately, if cpp-linter is run multiple times in a single CI event, then the PR review from cpp-linter (displayed as image) is overwritten by the last instance of cpp-linter that completed.

How to use clang-tidy going forward

First make sure clang-tidy is installed. Currently, we're using v14 of clang-format, so stick with that version.

sudo apt install clang-tidy-14

Do not attempt this on Windows because this lib cannot be compiled (or even cross-compiled). And we need a compilation database. WSL Ubuntu could be used instead, but the C/C++ std libs may not be exactly what native Linux env uses.

  1. generate a compilation database for the desired platform and/or Linux driver
    export RF24_DRIVER=SPIDEV
    mkdir build
    cmake -S . -B build
  2. run clang-tidy with the following args:
    clang-tidy-14 -p build RF24*.* utility/$RF24_DRIVER/*.*

Note

Reading the clang-tidy output might be tricky. Later versions of clang tools have improved the clang-tidy output, but you'll often find yourself referring to the clang-tidy docs for some errors.

- also enable generation of compilation database for all non-Arduino builds
- add clang-tidy config
- resolve implicit casting cases
- resolve `else` after `return` cases
- ignore false-positive about "memory allocated with zero size"
Copy link
Contributor

github-actions bot commented Sep 8, 2024

Memory usage change @ 48bab9b

Board flash % RAM for global variables %
arduino:avr:nano 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table
Board examples/GettingStarted
flash
% examples/GettingStarted
RAM for global variables
% examples/AcknowledgementPayloads
flash
% examples/AcknowledgementPayloads
RAM for global variables
% examples/ManualAcknowledgements
flash
% examples/ManualAcknowledgements
RAM for global variables
% examples/StreamingData
flash
% examples/StreamingData
RAM for global variables
% examples/MulticeiverDemo
flash
% examples/MulticeiverDemo
RAM for global variables
% examples/InterruptConfigure
flash
% examples/InterruptConfigure
RAM for global variables
% examples/scanner
flash
% examples/scanner
RAM for global variables
% examples/encodeRadioDetails
flash
% examples/encodeRadioDetails
RAM for global variables
%
arduino:avr:nano 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
arduino:samd:mkrzero 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
Click for full report CSV
Board,examples/GettingStarted<br>flash,%,examples/GettingStarted<br>RAM for global variables,%,examples/AcknowledgementPayloads<br>flash,%,examples/AcknowledgementPayloads<br>RAM for global variables,%,examples/ManualAcknowledgements<br>flash,%,examples/ManualAcknowledgements<br>RAM for global variables,%,examples/StreamingData<br>flash,%,examples/StreamingData<br>RAM for global variables,%,examples/MulticeiverDemo<br>flash,%,examples/MulticeiverDemo<br>RAM for global variables,%,examples/InterruptConfigure<br>flash,%,examples/InterruptConfigure<br>RAM for global variables,%,examples/scanner<br>flash,%,examples/scanner<br>RAM for global variables,%,examples/encodeRadioDetails<br>flash,%,examples/encodeRadioDetails<br>RAM for global variables,%
arduino:avr:nano,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0
arduino:samd:mkrzero,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0

@2bndy5 2bndy5 marked this pull request as ready for review September 9, 2024 08:15
@2bndy5 2bndy5 merged commit 25f1fbd into master Sep 13, 2024
80 checks passed
@2bndy5 2bndy5 deleted the clang-tidy branch September 13, 2024 09:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants