zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.61k stars 6.5k forks source link

Coding style problem, clang-format formatted code cannot pass CI. #51973

Closed RafaelLeeImg closed 1 year ago

RafaelLeeImg commented 1 year ago

Describe the bug clang-format result is different from the rules in CI, so code formatted by clang-format could not pass CI. Let human rember coding style or trial & error is not a systematic way for clean codes. I believe that I'm not the only one who has encountered this problem.

That space is added by clang-format.

https://github.com/zephyrproject-rtos/zephyr/actions/runs/3395127653/jobs/5646003818

clang-format --version
Debian clang-format version 14.0.6-2
-:89: ERROR:SPACING: space prohibited before that ':' (ctx:WxW)
#89: FILE: drivers/sensor/aht20/aht20.c:35:
+       uint8_t : 3;        /* bit [0:2] */
                ^

Please also mention any information which could help others to understand the problem you're facing:

To Reproduce Steps to reproduce the behavior:

  1. push a commit and trigger CI
  2. See error

Expected behavior Code pass CI

Impact Waste of time for multiple times.

Logs and console output

-:89: ERROR:SPACING: space prohibited before that ':' (ctx:WxW)
#89: FILE: drivers/sensor/aht20/aht20.c:35:
+       uint8_t : 3;        /* bit [0:2] */
                ^

-:91: ERROR:SPACING: space prohibited before that ':' (ctx:WxW)
#91: FILE: drivers/sensor/aht20/aht20.c:37:
+       uint8_t : 1;        /* bit [4] */
                ^

-:92: ERROR:SPACING: space prohibited before that ':' (ctx:WxW)
#92: FILE: drivers/sensor/aht20/aht20.c:[38](https://github.com/zephyrproject-rtos/zephyr/actions/runs/3395127653/jobs/5646003818#step:9:39):
+       uint8_t : 2;        /* bit [5:6], AHT20 datasheet v1.1 removed 2 mode bits */
                ^

-:143: WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#1[43](https://github.com/zephyrproject-rtos/zephyr/actions/runs/3395127653/jobs/5646003818#step:9:44): FILE: drivers/sensor/aht20/aht20.c:89:
+   if (0 != ret) {

-:149: WARNING:LINE_SPACING: Missing a blank line after declarations
#149: FILE: drivers/sensor/aht20/aht20.c:95:
+       static uint8_t const initialize_seq[] = {AHT20_CMD_INITIALIZE};
+       ret = i2c_write_dt(&drv_data->bus, initialize_seq, sizeof(initialize_seq));

-:1[50](https://github.com/zephyrproject-rtos/zephyr/actions/runs/3395127653/jobs/5646003818#step:9:51): WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#150: FILE: drivers/sensor/aht20/aht20.c:96:
+       if (0 != ret) {

- total: 3 errors, 4 warnings, 197 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES C99_COMMENT_TOLERANCE COMPLEX_MACRO CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME DT_SCHEMA_BINDING_PATCH DT_SPLIT_BINDING_PATCH ENOSYS FILE_PATH_CHANGES IS_ENABLED_CONFIG MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PREFER_KERNEL_TYPES PREFER_SECTION PRINTK_WITHOUT_KERN_LEVEL REPEATED_WORD SPDX_LICENSE_TAG SPLIT_STRING TRAILING_SEMICOLON UNDOCUMENTED_DT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.
Error: Process completed with exit code 1.

Environment (please complete the following information):

Additional context Is is an existing problem lasts for at least a year.

stephanosio commented 1 year ago

clang-format result is different from the rules in CI, so code formatted by clang-format could not pass CI.

This in itself is not a bug. We have never made any guarantees that the style defined by the clang-format file will 100% match the Zephyr coding style and guidelines.

For now, clang-format support is more or less experimental and one should not expect it to be a one-stop solution to all coding formatting issues -- additional human interventions may be necessary.

Also, please see the related discussions in the following issues:

stephanosio commented 1 year ago

cc @gmarull

RafaelLeeImg commented 1 year ago

At least there shall be some way that let user to setup the coding style checker at their local computer, is there any way to do that? I didn't find any related documents.

https://docs.zephyrproject.org/latest/contribute/guidelines.html#coding-style

stephanosio commented 1 year ago

At least there shall be some way that let user to setup the coding style checker at their local computer, is there any way to do that? I didn't find any related documents.

Did you see The Linux kernel GPL-licensed tool checkpatch is used to check coding style conformity.?

RafaelLeeImg commented 1 year ago

OK, now I know how to use that. Shall those included in Zephyr documents?

./scripts/checkpatch.pl 0001-sensors-Add-AHT20-temperature-humidity-sensor.patch

By the way, it's showing another false positive warning, it's not lables, it's bit field.

+typedef union {

0001-sensors-Add-AHT20-temperature-humidity-sensor.patch:108: WARNING:INDENTED_LABEL: labels should not be indented
#108: FILE: drivers/sensor/aht20/aht20.c:35:
+       uint8_t: 3;     /* bit [0:2] */

Related code is:

typedef union {
    struct {
        uint8_t: 3;     /* bit [0:2] */
        uint8_t cal_enable : 1; /* bit [3] */
        uint8_t: 1;     /* bit [4] */
        uint8_t: 2;     /* bit [5:6], AHT20 datasheet v1.1 removed 2 mode bits */
        uint8_t busy : 1;   /* bit [7] */
    };
    uint8_t all;
} __attribute__((__packed__)) aht20_status;
stephanosio commented 1 year ago

OK, now I know how to use that. Shall those included in Zephyr documents?

The instruction is right there in the documentation you linked ...

By the way, it's showing another false positive warning, it's not lables, it's bit field.

This will be fixed by https://github.com/zephyrproject-rtos/zephyr/pull/51975

gmarull commented 1 year ago

Unless there's a clang-format option to match checkpatch expectations, I think the best is to patch checkpatch or ignore the failure.

chrta commented 1 year ago

It should be possible to fix the clang format config. See setting BitFieldColonSpacing (BitFieldColonSpacingStyle) that is available starting from clang-format 12: https://clang.llvm.org/docs/ClangFormatStyleOptions.html This should be set to After.

RafaelLeeImg commented 1 year ago

Thanks. I tried offline, it works. I have created a pull-request #52268

RafaelLeeImg commented 1 year ago

Pull request merged #52268 This problem is solved.