clang-format: improve and update

[gmarull]

The number of options available on clang-format has become quite
extensive over-time, making a dumped file difficult to read or maintain.
In this patch, the .clang-format has been re-written by inheriting from
LLVM (default) since most of the options can be re-used to match the
Zephyr/Linux coding style conventions.

[carlescufi]

I haven’t tested it but the changes look reasonable.

[ppryga-nordic]

I didn't compare inherited options. I assume that you have tested it and results are exactly the same or even better due to enable options that were commented out for backward compatibility. Am I right with my assumption?

[teburd]

I tried this out on the existing code base and, at least personally, I thought it did a really nice job.

It aligned macro and function arguments that were previously unaligned. It removed extraneous spaces, which there were surprisingly things like

uint32_t x =  123;

Where the double spacing after the = would be fixed to a single space.

It generally made things more, not less, readable. I didn't run checkpatch after, that might be an interesting bout to see if they agree.

[nashif]

getting:

clang-format kernel/sem.c
/home/nashif/Work/zephyrproject/zephyr/.clang-format:63:20: error: unknown enumerated scalar
SpaceBeforeParens: ControlStatementsExceptControlMacros
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Error reading /home/nashif/Work/zephyrproject/zephyr/.clang-format: Invalid argument

clang-format version 12.0.1 (Fedora 12.0.1-1.fc34)

[gmarull]

getting:

clang-format kernel/sem.c
/home/nashif/Work/zephyrproject/zephyr/.clang-format:63:20: error: unknown enumerated scalar
SpaceBeforeParens: ControlStatementsExceptControlMacros
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Error reading /home/nashif/Work/zephyrproject/zephyr/.clang-format: Invalid argument

clang-format version 12.0.1 (Fedora 12.0.1-1.fc34)

Commented out this option (available since 13.0)

[erwango]

I'm not sure how to do this and avoiding bike-shading but in order to help getting a consensus on the change and not coming back on it later, would it be possible to demonstrate the result if this change, so each one can have a chance to give his opinion and commonly agree on the result?
For sure, one can do it in his side and see if happy with the result, but demonstration result depends on the sample file used. Having a draft PR which uses this change to comment would be nice, IMO.

[nashif]

@gmarull can this PR also remove .uncrustify.cfg and replace uncrustify with clang-format in the docs as well, related bug #41281 dealing with this assigned to you to close in one shot :)

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[teburd]

This isn't stale, and I still care

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[cfriedt]

It generally made things more, not less, readable. I didn't run checkpatch after, that might be an interesting bout to see if they agree.

checkpatch.pl is both a formatting tool and linter. The clang solution uses clang-tidy as the linter and clang-format as the formatter. As long as clang-format and checkpatch.pl agree on formatting, we should be good.

It should be noted that the clang tooling actually uses LLVM as the C / C++ parser, so it's quite accurate as it's derived from the AST. IIRC, checkpatch.pl might only be suitable for C, so we may eventually want to replace it with clang-format and also introduce clang-tidy into CI.

Of course, there is also scan-build for static analysis, which uses LLVM and leverages the AST as well.

[gmarull]

It generally made things more, not less, readable. I didn't run checkpatch after, that might be an interesting bout to see if they agree.

checkpatch.pl is both a formatting tool and linter. The clang solution uses clang-tidy as the linter and clang-format as the formatter. As long as clang-format and checkpatch.pl agree on formatting, we should be good.

It would be nice to introduce clang-tidy as well. The only issue I have observed with clang-format/checkpatch.pl is within macros:

	static const struct jm101_config jm101_config_##i = {                  \
		...							       \
	};                                                                     \
                                                                               \ /* <- clang-format starts line with spaces here, not tab, checkpatch complains about leading spaces */
	DEVICE_DT_INST_DEFINE(i, jm101_init, NULL, &jm101_data_##i,            \

However, I guess we could patch checkpatch to ignore those edge cases. Our abuse of nested macros can confuse clang-format as well, for example,

	static const struct jm101_config jm101_config_##i = {                  \
		.uart = DEVICE_DT_GET(DT_INST_PHANDLE(i, uart)),               \
		.addr = DT_INST_PROP(i, address),                              \
		IF_ENABLED(CONFIG_JM101_TRIGGER,                               \
			   (.touch = GPIO_DT_SPEC_INST_GET_OR(i, touch_gpios,  \
							      {}), )) /* */    \ /* comment added to force line break here, otherwise clang-format doesn't work well */
	};                                                                     \
                                                                               \
	DEVICE_DT_INST_DEFINE(i, jm101_init, NULL, &jm101_data_##i,            \

But overall, I had a good experience with it, specially when refactoring code.

[ppryga-nordic]

I'm also in favor to use clang-format. From time to time e.g. in BT Controller's code I observe that the formatting is a bit different than done with clang-format. That introduces additional changes to PRs that not always are necessary or welcome by reviewers.
At the end of the day, the more it is used the less of these situations happens.

[cfriedt]

It would be nice to introduce clang-tidy as well. The only issue I have observed with clang-format/checkpatch.pl is within macros:

@gmarull - that mirrors my experiences as well. It's probably something that is worth fixing upstream on behalf of the Zephyr project if we were to adopt clang-format.

[gmarull]

@gmarull - that mirrors my experiences as well. It's probably something that is worth fixing upstream on behalf of the Zephyr project if we were to adopt clang-format.

Agreed, would be nice to have these small issues fixed upstream.

[teburd]

Yes please

[carlescufi]

+1 to clang-format, @nashif and @MaureenHelm ping