Skip to content

makefiles/clang-tidy: initial support#16509

Merged
maribu merged 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_clang_tidy_target
Aug 31, 2021
Merged

makefiles/clang-tidy: initial support#16509
maribu merged 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_clang_tidy_target

Conversation

@fjmolinas
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas commented May 28, 2021

Contribution description

This PR adds a make target to run clang-tidy. By default the considered source files will those that have been changed regarding to BASE_BRANCH set to master by default.

Testing procedure

On a branch different than master run TOOLCHAIN=llvm make -C tests/event_timeout_ztimer/ clang-tidy

BASE_BRANCH=upstream/master TOOLCHAIN=llvm make -C tests/event_timeout_ztimer/ clang-tidy
64 warnings generated.
127 warnings generated.
192 warnings generated.
328 warnings generated.
404 warnings generated.
/home/francisco/workspace/RIOT/sys/event/periodic_timeout.c:9:1: warning: #includes are not sorted properly [llvm-include-order]
#include "kernel_defines.h"
^        ~~~~~~~~~~~~~~~~~~
         "event/timeout.h"
/home/francisco/workspace/RIOT/sys/include/event/timeout.h:157:20: warning: unused function 'event_periodic_timeout_start' [clang-diagnostic-unused-function]
static inline void event_periodic_timeout_start(event_periodic_timeout_t *event_timeout,
                   ^
/home/francisco/workspace/RIOT/sys/include/event/timeout.h:174:20: warning: unused function 'event_periodic_timeout_stop' [clang-diagnostic-unused-function]
static inline void event_periodic_timeout_stop(event_periodic_timeout_t *event_timeout)
                   ^
/home/francisco/workspace/RIOT/tests/event_timeout_ztimer/main.c:26:1: warning: #includes are not sorted properly [llvm-include-order]
#include "test_utils/expect.h"
^        ~~~~~~~~~~~~~~~~~~~~~
         "event.h"
/home/francisco/workspace/RIOT/tests/events/main.c:25:1: warning: #includes are not sorted properly [llvm-include-order]
#include "test_utils/expect.h"
^        ~~~~~~~~~~~~~~~~~~~~~
         "event.h"
/home/francisco/workspace/RIOT/tests/events/main.c:201:48: warning: 500 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
    event_timeout_set(&event_timeout_canceled, 500 * US_PER_MS);
                                               ^
/home/francisco/workspace/RIOT/tests/events/main.c:201:48: warning: 500 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
Suppressed 397 warnings (397 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

@fjmolinas fjmolinas requested a review from bergzand May 28, 2021 10:00
@github-actions github-actions bot added the Area: build system Area: Build system label May 28, 2021
@aabadie aabadie added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label May 30, 2021
aabadie
aabadie reviewed Jun 1, 2021
View reviewed changes
Makefile.include
Comment on lines +836 to +837
# inlcude clang-tidy
include $(RIOTMAKE)/clang_tidy.inc.mk
Copy link
Copy Markdown
Contributor

@aabadie aabadie Jun 1, 2021

Choose a reason for hiding this comment

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

Why not only include this if llvm toolchain is used (there's also a typo btw):

Suggested change
# inlcude clang-tidy
include $(RIOTMAKE)/clang_tidy.inc.mk
# include clang-tidy if llvm toolchain is used
ifeq (llvm,$(TOOLCHAIN))
include $(RIOTMAKE)/clang_tidy.inc.mk
endif

Then there's no need for the error message in clang_tidy.inc.mk (that I would rename clang-tidy.inc.mk but that's a matter of taste and I won't insist)

Copy link
Copy Markdown
Contributor Author

@fjmolinas fjmolinas Jun 1, 2021

Choose a reason for hiding this comment

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

I thought it made the target more visible that way.

@fjmolinas fjmolinas added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 7, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@fjmolinas fjmolinas mentioned this pull request Aug 31, 2021
Merged
maribu
maribu approved these changes Aug 31, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Works as advertised and is pretty useful :-)

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 31, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Aug 31, 2021

Without compile-tests another round of Murdock shouldn't take too long, and ruling out any Makefile breakage is likely worth the effort.

@maribu maribu merged commit a3f880c into RIOT-OS:master Aug 31, 2021
@fjmolinas fjmolinas deleted the pr_clang_tidy_target branch August 31, 2021 13:28
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Aug 31, 2021

Thanks for the review!

@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie left review comments

@maribu maribu maribu approved these changes

@bergzand bergzand Awaiting requested review from bergzand

Assignees

No one assigned

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2021.10

Development

Successfully merging this pull request may close these issues.

5 participants

@fjmolinas @maribu @aabadie @benpicco @MrKevinWeiss