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

Replace header guards style with #pragma once #102298

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Feb 1, 2025

An upcoming core meeting will touch on using #pragma once instead of traditional header guards. This draft is meant to accompany that future discussion, as it showcases the overall scope of a total conversion. While this would affect virtually every non-thirdparty header in the repo, the scope is isolated to such an extent that it can be safely excluded via .git-blame-ignore-revs. Changes to non-header files — builders, pre-commit, etc — were handled in a separate commit which would not be excluded. Does not convert headers for *-so_wrap.h files (generated externally), nor ObjC files (guards shouldn't be there in the first place, but removing them is out-of-scope; handled in #101174 instead).

EDIT: Now also includes ObjC files, as their compilers also handle #pragma once just fine. Having them included means that the logic for checking header guards has been reduced significantly.

@AThousandShips
Copy link
Member

Should probably be handled safely generally but just to confirm one area of potential issue:
Does this work correctly in SCU?

@Chaosus
Copy link
Member

Chaosus commented Feb 1, 2025

Heh, I remember the time when @vnen said it is wrong idea: #18143 (comment)

@Repiteo
Copy link
Contributor Author

Repiteo commented Feb 1, 2025

Does this work correctly in SCU?

Verified locally that it does work!

Heh, I remember the time when vnen said it is wrong idea: #18143 (comment)

I understand the hesitation for adding non-standard functionality, but I can make an exception here for a few reasons:

  • #pragma once is ubiquitous to the point that it might as well be part of the standard
  • All of our tested buildsystems support #pragma once, so it's a non-blocker for our purposes
  • We're already using #pragma once in several third-party files. If strict-conformance was a concern, I imagine there'd be instances of developers opening issues to request the third-party code use header guards instead. Seeming as that has never been the case (to my knowledge), it's reasonable to conclude this isn't a real-world problem

@fire fire changed the title Style: Replace header guards with #pragma once Replace header guards style with #pragma once Feb 2, 2025
@lawnjelly
Copy link
Member

lawnjelly commented Feb 3, 2025

Does this work correctly in SCU?

Yeah it's fine I've been using #pragma once for years with SCU builds.

I'm personally in favour of #pragma once BTW, I have a vague memory Juan wasn't convinced of need for change last time we discussed, but he may be fine with it if enough people in favour.

As we said last time:

  • It's available on all compilers these days (has been for decades I think we looked this up last time)
  • It makes one less think to screw up if there are duplicate defines in different files
  • Less verbose and less cognitive effort (particularly if you have other #ifdef sections, or namespaces)
  • It's one of the (few 😄 ) extensions to c++ MS got right
  • Easy to revert to #ifdef guards if a rare file needs to be given specialist treatment

@Repiteo
Copy link
Contributor Author

Repiteo commented Feb 4, 2025

If explicit affirmation is necessary, then I'll echo that I'm very much in favor of this as well. In addition to the points lawnjelly already mentioned, it'll be MUCH less of a headache to enforce in terms of hooks/tools. The existing methods we have now are pretty hacky, because they have to account for the exact situations #pragma once avoid: duplicate names & unorthodox positioning.

@Ivorforce
Copy link
Contributor

Just a thought, does this observably affect compile time?

@YYF233333
Copy link
Contributor

Just a thought, does this observably affect compile time?

I tested this about half a month ago using scons dev_mode=yes dev_build=yes on Windows with MSVC and didn't observe any noticeable performance improvement (both full rebuild and no-op).

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Changes to tests/create_test.py look good.

@Repiteo Repiteo marked this pull request as ready for review February 13, 2025 17:11
@Repiteo Repiteo requested review from a team as code owners February 13, 2025 17:11
@Repiteo Repiteo removed request for a team February 13, 2025 17:12
@Repiteo Repiteo modified the milestones: 4.x, 4.5 Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants