Skip to content
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

style: Inconsistent macro names changed #83061

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rruuaanng
Copy link
Collaborator

Fix incorrect header file pre-macro names in
include/zephyr/dt-bindings.

gmarull
gmarull previously approved these changes Dec 17, 2024
decsny
decsny previously approved these changes Dec 17, 2024
@kartben
Copy link
Collaborator

kartben commented Dec 19, 2024

@rruuaanng needs rebase

@rruuaanng rruuaanng dismissed stale reviews from decsny and gmarull via cdee5bc December 19, 2024 11:47
@rruuaanng rruuaanng force-pushed the fix-pre-macro branch 2 times, most recently from cdee5bc to 787fe8a Compare December 19, 2024 11:54
decsny
decsny previously approved these changes Dec 19, 2024
kartben
kartben previously approved these changes Dec 19, 2024
@@ -3,8 +3,8 @@
*
* SPDX-License-Identifier: Apache-2.0
*/
#ifndef ZEPHYR_INCLUDE_DT_BINDINGS_CLOCK_NPCK_FIU_QSPI_H_
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha, so that is where the file comes from..

de-nordic
de-nordic previously approved these changes Dec 19, 2024
@kartben
Copy link
Collaborator

kartben commented Dec 24, 2024

@rruuaanng conflicts once again and needs rebase:) thanks!

@kartben
Copy link
Collaborator

kartben commented Dec 24, 2024

@soburi @ajf58 can you please look into the CI failure? Not sure why this wasn't caught on main, but this looks related to yesterday's Pico2 changes? Thanks!

@soburi
Copy link
Member

soburi commented Dec 25, 2024

Is this rebased from before #83343 commits?
I think this PR is a fix for the include guard,
but for
include/zephyr/dt-bindings/pinctrl/rpi-pico-rp2040-pinctrl.h
it looks like not only it.
It seems also contains revert #83343 changes.

@rruuaanng
Copy link
Collaborator Author

rruuaanng commented Dec 25, 2024

@kartben I have fixed it, I will raise a PR later, hope it helps.

Edit

diff

-#define RP2XXX_PINMUX(pin_num, alt_func) (pin_num << RP2_PIN_NUM_POS | alt_func << RP2_ALT_FUNC_POS)
+#define RP2XXX_PINMUX(pin_num, alt_func) ((pin_num << RP2_PIN_NUM_POS) \
+                                        | (alt_func << RP2_ALT_FUNC_POS))

It looks like the parser is parsing << as a symbol instead of an operator.

@ajf58
Copy link
Contributor

ajf58 commented Dec 27, 2024

Relates to #80781 .

@rruuaanng In the commit message:

Fix incorrect header file pre-macro names in
'include/zephyr/dt-bindings'.

  1. I've never seen include guards referred to as "pre-macro names", and it's a bit close to "predefined macros" which is something else entirely. Please consider rephrasing this.
  2. When you say "incorrect", what do you mean by that? I don't think that the project has any documentation that defines this, and it's challenging for contributors to know what's right or wrong without this information. Can you refer to it in the PR and/or commit message, or update the documentation?

@rruuaanng
Copy link
Collaborator Author

rruuaanng commented Dec 28, 2024

Congratulations on discovering an interesting point. In fact, this is just a convention in Zephyr. For some challenging subsystems, typical contributors and maintainers, who usually have extensive experience, choose to follow the project's conventions. However, there are currently some subsystems synchronized from authoritative upstream projects, such as Xen. If the include guard macro names for these subsystems were to follow the convention, they would also be incorrect. I believe that when fixing other macros that need synchronization, we should not include the correct or conventional names in the commit messages (this is usually the case).

But I'm willing to modify the commit information. It does look a little vague.

@ajf58
Copy link
Contributor

ajf58 commented Dec 28, 2024

@rruuaanng

Making things consistent is great, following a consistent pattern is also great, no disagreement at all from me.

We do need to be mindful, however, that we don't lean hard on unwritten rules of right/wrong, correct/incorrect, and I feel we can use these kinds of PRs as opportunities to improve the documentation. The implicit rule is "swap include/zephyr for zephyr/include, convert non-alpha characters to underscores, capitalize, add a _H_ suffix." Quite a mouthful, and the swapping step is a bit a of mystery to me.

@rruuaanng
Copy link
Collaborator Author

@ajf58 Since that’s the case, are you interested in supplementing the documentation? Perhaps these guidelines should be included in the contribution guide?

@ajf58
Copy link
Contributor

ajf58 commented Dec 29, 2024

are you interested in supplementing the documentation?

In general, yes.

Perhaps these guidelines should be included in the contribution guide?

I agree, this matches one of my suggestions in #80781 .

Currently the documentation says follow the Linux kernel style guides, which does not specify what pattern include guards take. #define BANANA_DISCO_UNIQUE_ANYTHING_WITHOUT_SUFFIX would be valid, if not intuitive.

If there's some agreement about what is The One True include guard style, I'm happy to update the docs, but as mentioned in #80781 , some folders have multiple conflicting styles, and there's no way to tell which, if any, is correct.

Fix incorrect header file pre-macro names in
'include/zephyr/dt-bindings'.

Signed-off-by: James Roy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Coding Guidelines Coding guidelines and style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants