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

Include guard rules (if mandated) should be documented and handled by software #80781

Open
ajf58 opened this issue Nov 2, 2024 · 5 comments
Open
Assignees
Labels
area: Documentation Enhancement Changes/Updates/Additions to existing features

Comments

@ajf58
Copy link
Contributor

ajf58 commented Nov 2, 2024

Is your enhancement proposal related to a problem? Please describe.

At review-time, a disproportionate amount of time is spent by contributors and reviewers discussing include guards. There are a number of problems with this:

  1. This is not documented in the Contribution Guidelines.
    The guidelines defer to the Linux kernel coding style, which does not define or enforce the style. Contributors can't know this expectation in advance of doing their work. This is a problem because it is demotivating for Contributors, and a problem for the project because it slows development down.
  2. The codebase is inconsistent.
    This in itself would not be a problem if (1) was in effect, but there's also no way for a Contributor to look at prior-art and determine what they've done is wrong.

Describe the solution you'd like

  1. Defer to the kernel coding style.
    Perhaps the easiest solution. Unless there's a technical reason that the Contributor's choice of include guard is a problem, leave it alone.
  2. If there's a mandated requirement,
    1. Define and Document it.
      Allow Contributors to know what they're expected to do before they invest their own time and effort in the codebase. They may decide, based on this, whether they want to contribute at all.
    2. Provide hooks for checking it:
      1. A client side pre-commit (or pre-push) hook could determine whether a changed file is a header, and if the include guard matches the defined style.
      2. Extend the Compliance Checks to also check this.
        Contributors may not be willing or able to run the client-side hooks, so check this.
    3. Provide scripts for fixing it:
      1. This is akin to the code-formatting hints already provided by CI. Hint at a solution, and provide a script to do the work.
    4. Define and implement a plan for the existing codebase.
      Are existing files going to remain incosistent, or will they be fixed in a treewide tidy up, or as-and-when they are touched?

Describe alternatives you've considered

Continue the status-quo, (existing codebase remains inconsistent, add-hoc review-time requests to change files occur).

Additional context

A good example of the self-inconsistent codebase can be seen in a single directory:

$ grep -hr -m1 '#ifndef' include/zephyr/dt-bindings/pinctrl/ | sort

which presents ~8 different conventions.

@ajf58 ajf58 added the Enhancement Changes/Updates/Additions to existing features label Nov 2, 2024
@kartben kartben added area: Documentation Process Tracked by the process WG labels Nov 2, 2024
@github-project-automation github-project-automation bot moved this to To do in Process Nov 2, 2024
@jukkar
Copy link
Member

jukkar commented Nov 3, 2024

Just fyi, I found this old PR #9990 (comment) that points to this document about naming the guards https://google.github.io/styleguide/cppguide.html#The__define_Guard

@ajf58
Copy link
Contributor Author

ajf58 commented Nov 9, 2024

Right, but the full comment on the link:

I would not go all the way and add the path, but ZEPHYR_ADC_H should do it.

Which, even with that said, isn't what happens: many times the include guard is of the form <PROJECT>_<path-with-zephyr-omitted>_<FILE>_H_

I would start a document (or use an existing one) to document those and make it part of the Zephyr style and naming conventions.

Which hasn't happened, and is the reason I've raised this ticket.

@ajf58
Copy link
Contributor Author

ajf58 commented Nov 26, 2024

And sometimes we end up with no include guards at all (see #81196) .

@keith-zephyr keith-zephyr moved this from To do to In progress in Process Dec 10, 2024
@ajf58
Copy link
Contributor Author

ajf58 commented Dec 17, 2024

Related to #83117

@keith-zephyr
Copy link
Contributor

Process WG on 2024-12-11 decided this didn't need discussion by the Process WG.

Please submit a specific proposal with language you would like added to the coding guidelines.

@keith-zephyr keith-zephyr removed the Process Tracked by the process WG label Jan 8, 2025
@keith-zephyr keith-zephyr moved this from In progress to Done in Process Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Documentation Enhancement Changes/Updates/Additions to existing features
Projects
Status: Done
Development

No branches or pull requests

5 participants