Use #pragma once instead of handcrafted header guards
#669
Conversation
There was a problem hiding this comment.
Reviewed 120 of 120 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)
Documentation/devel/coding-style.rst line 109 at r1 (raw file):
#. Gramine's headers. #. Assignments may be aligned when assigning some structurized data (e.g. struct
I would move the new point before this line, because previous talks about includes too (albeit in a different context)
LibOS/include/arch/x86_64/gramine_entry_api.h line 14 at r1 (raw file):
*/ #ifndef GRAMINE_ENTRY_API_H_
I Would make this file an exception. It's included from outside, so who knows what magic with hard/soft links people will do on it.
Pal/include/pal/pal.h line 17 at r1 (raw file):
#include <stdnoreturn.h> // TODO: fix this (but see x86_64/pal.h)
Please use full repo path, I don't even know which file this talks about (there is only one pal.h, this one)
There was a problem hiding this comment.
Reviewable status: 115 of 120 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
Documentation/devel/coding-style.rst line 109 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I would move the new point before this line, because previous talks about includes too (albeit in a different context)
Done.
LibOS/include/arch/x86_64/gramine_entry_api.h line 14 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I Would make this file an exception. It's included from outside, so who knows what magic with hard/soft links people will do on it.
Done.
Pal/include/pal/pal.h line 17 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please use full repo path, I don't even know which file this talks about (there is only one
pal.h, this one)
Oh, that's actually a mistake on my side in the header name. But I can also extend the whole path in addition to fixing it.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)
common/include/api.h line 17 at r2 (raw file):
#endif #define INSIDE_API_H
Don't we want to move it just after standard includes? (assert can be our own)
LibOS/include/arch/x86_64/gramine_entry_api.h line 15 at r2 (raw file):
/* We don't use `#pragma once` here because this header is used by our custom glibc build, and * glibc's codebase and buildsystem is a mess. */
It's also a public api - people can build their own libraries against it (it's not only our glibc).
There was a problem hiding this comment.
Reviewable status: 119 of 120 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
common/include/api.h line 17 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Don't we want to move it just after standard includes? (assert can be our own)
Ah, right, overlooked the assert.
LibOS/include/arch/x86_64/gramine_entry_api.h line 15 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
It's also a public api - people can build their own libraries against it (it's not only our glibc).
But this works in normal cases. We're only afraid about glibc because it's known for unholy hacks.
There was a problem hiding this comment.
Reviewed 115 of 120 files at r1, 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
Pal/include/pal/pal.h line 17 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Oh, that's actually a mistake on my side in the header name. But I can also extend the whole path in addition to fixing it.
Fix what? What is going on here? The referred file doesn't seem to have any explanations too.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)
LibOS/include/arch/x86_64/gramine_entry_api.h line 15 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
But this works in normal cases. We're only afraid about glibc because it's known for unholy hacks.
Quoting commit message: Cons of `#pragma once`: (...) compiler-specific behavior in cases like including the same file through different {soft,hard}links.
This file can be used outside of Gramine, so we don't know what voodo people will use.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)
Pal/include/pal/pal.h line 17 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Fix what? What is going on here? The referred file doesn't seem to have any explanations too.
It says: This header is usable only inside pal.h (due to a cyclic dependency). So, that's the explanation why this hack is currently needed.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
LibOS/include/arch/x86_64/gramine_entry_api.h line 15 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Quoting commit message:
Cons of `#pragma once`: (...) compiler-specific behavior in cases like including the same file through different {soft,hard}links.
This file can be used outside of Gramine, so we don't know what voodo people will use.
I'd say it's their own choice to do such hacks (and you really need to do crazy things to notice any problems with #pragma once). Anyways, this works, it's only about the explanation comment now, and IMO we wouldn't change it if not for glibc.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)
LibOS/include/arch/x86_64/gramine_entry_api.h line 15 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd say it's their own choice to do such hacks (and you really need to do crazy things to notice any problems with
#pragma once). Anyways, this works, it's only about the explanation comment now, and IMO we wouldn't change it if not for glibc.
But we never changed it due to glibc in the first place. It's our own header used in our own glibc patches, so there is no risk of failures there.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)
Pal/include/pal/pal.h line 17 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
It says:
This header is usable only inside pal.h (due to a cyclic dependency). So, that's the explanation why this hack is currently needed.
Ok, got it. Several lines below (hidden in Reviewable), we have #include "pal-arch.h" -- this TODO comment and macro definition are for that header. Confusing, but whatever.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
LibOS/include/arch/x86_64/gramine_entry_api.h line 15 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
But we never changed it due to glibc in the first place. It's our own header used in our own glibc patches, so there is no risk of failures there.
Hmm, I changed it for the glibc ;)
There is a risk, we are not sure what glibc does with these files during build (may copy them a few times, or something) and it also may start causing troubles in new glibcs. If not for glibc I wouldn't really change it.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)
LibOS/include/arch/x86_64/gramine_entry_api.h line 15 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Hmm, I changed it for the glibc ;)
There is a risk, we are not sure what glibc does with these files during build (may copy them a few times, or something) and it also may start causing troubles in new glibcs. If not for glibc I wouldn't really change it.
I don't understand why the argument is different for glibc and for other usages (by external users, outside of our control).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
LibOS/include/arch/x86_64/gramine_entry_api.h line 15 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I don't understand why the argument is different for glibc and for other usages (by external users, outside of our control).
The other users use this header in their projects intentionally, while in glibc it's us who glue it into glibc codebase and outside of glibc devs control. If someone else intentionally uses this header in their project and then does some crazy hacks then if it breaks, it's on their own request.
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)
a discussion (no related file):
I found at least one header file that didn't have the guard: enclave_ocalls.h. We probably have a couple of other header files like this, that don't have any protection at all.
Could you please try to find them and add #pragma once to them?
There was a problem hiding this comment.
Reviewable status: 120 of 131 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I found at least one header file that didn't have the guard:
enclave_ocalls.h. We probably have a couple of other header files like this, that don't have any protection at all.Could you please try to find them and add
#pragma onceto them?
Good catch! Found quite a lot of them...
Anyways, fixed, hopefully all.
There was a problem hiding this comment.
Reviewed 1 of 5 files at r2.
Reviewable status: 120 of 131 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
Documentation/devel/coding-style.rst line 99 at r2 (raw file):
#. Use ``#pragma once`` for include guards.
This could use some explanation/rationale, like that in PR description, for people who would like to ever revisit the decision. We know it's non-standard, but it's supported by the compilers we're interested in, and we think it's less error-prone.
There was a problem hiding this comment.
Reviewable status: 120 of 131 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
Documentation/devel/coding-style.rst line 99 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This could use some explanation/rationale, like that in PR description, for people who would like to ever revisit the decision. We know it's non-standard, but it's supported by the compilers we're interested in, and we think it's less error-prone.
I considered it, but then decided that this guide should be concise and quick to read / look up a specific rule, and explaining this choice requires at least a paragraph of text. Also, other rules here are not explained either.
I guess you can always find this commit or this PR if you want to check the arguments in the future.
There was a problem hiding this comment.
Reviewable status: 120 of 131 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Documentation/devel/coding-style.rst line 99 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I considered it, but then decided that this guide should be concise and quick to read / look up a specific rule, and explaining this choice requires at least a paragraph of text. Also, other rules here are not explained either.
I guess you can always find this commit or this PR if you want to check the arguments in the future.
OK, fair enough.
There was a problem hiding this comment.
Reviewed 11 of 11 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners
Cons of `#pragma once`:
- not in C standard
- compiler-specific behavior in cases like including the same file
through different {soft,hard}links, or multiple copies of the
same header in the repo
Pros:
- less boilerplate
- works well in practice
- no need to update it when moving/renaming a header file
- handcrafted header guards are error prone (easy to have
non-matching `#ifndef` and `#define`, easy to collide with other
file header guards, and especially prone to copy-paste errors)
The cases with {soft,hard}links and using multiple copies of the same
headers are rather exotic and we don't ever plan to have them in our
codebase.
Overall: `#pragma once` works well in practice and it's much more likely
that we'll introduce a bug with handcrafted include guards than with
`#pragma once`.
Signed-off-by: Michał Kowalczyk <[email protected]>
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
There was a problem hiding this comment.
Reviewed 11 of 11 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewable status:
Description of the changes
Cons of
#pragma once:Pros:
#ifndefand#define, easy to collide with other file header guards, and especially prone to copy-paste errors)The cases with {soft,hard}links and using multiple copies of the same headers are rather exotic and we don't ever plan to have them in our codebase.
Overall:
#pragma onceworks well in practice and it's much more likely that we'll introduce a bug with handcrafted include guards than with#pragma once.How to test this PR?
CI.
This change is