Skip to content

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

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

Closed
RafaelLeeImg opened this issue Nov 4, 2022 · 10 comments
Closed

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

RafaelLeeImg opened this issue Nov 4, 2022 · 10 comments
Labels
Enhancement Changes/Updates/Additions to existing features

Comments

@RafaelLeeImg
Copy link
Contributor

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:

  • What target platform are you using?
    ARM Cortex-M, it is not relevant to this issue
  • What have you tried to diagnose or workaround this issue?
    Modify code by hand, ignore the formatted code of clang-format
  • Is this a regression? If yes, have you been able to "git bisect" it to a
    specific commit?
  • ...

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):

  • OS: Linux
  • Toolchain
    Debian clang-format version 14.0.6-2
  • Commit SHA or Version used
    RafaelLeeImg@c953e81

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

@RafaelLeeImg RafaelLeeImg added the bug The issue is a bug, or the PR is fixing a bug label Nov 4, 2022
@stephanosio
Copy link
Member

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 stephanosio added Enhancement Changes/Updates/Additions to existing features area: Code Style and removed bug The issue is a bug, or the PR is fixing a bug labels Nov 4, 2022
@stephanosio
Copy link
Member

cc @gmarull

@RafaelLeeImg
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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 #51975

@gmarull
Copy link
Member

gmarull commented Nov 7, 2022

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

@chrta
Copy link
Collaborator

chrta commented Nov 10, 2022

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
Copy link
Contributor Author

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

@RafaelLeeImg
Copy link
Contributor Author

Pull request merged #52268
This problem is solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

4 participants