commit 31dab65916568140c3921ef1599e002315ca95bd Author: Mateusz Guzik <mjg@FreeBSD.org> Date: Sun Mar 12 19:31:27 2023 +0000 vfs: use __enum_uint8 for vtype and vstate Reviewed by: Differential Revision: commit 73129bc43e901a063a0a2fa3ec024a53b773a83a Author: Mateusz Guzik <mjg@FreeBSD.org> Date: Sun Mar 12 19:29:20 2023 +0000 Support byte-sized enums To that end add __enum_uint8_decl and __enum_uint8. By default enums take 4 bytes, but vast majority of them have values which would fit in a byte. One can try to workaround the problem by using bitfields, like so: enum some_small_enum foo:8; but that's ugly and runs into trouble when using atomic_load (you can't take an address of a bitfield, even if it is sized to a multiply of a byte). Both gcc 13 and clang support specifying the size, and for older variants one can fallback to the "packed" attribute. Names are mangled in order to avoid mix use with plain enum. Reviewed by: Differential Revision: https://reviews.freebsd.org/D39031 commit 93f3e9dba8b886058964c7acbcc6ce170a1d1f6a Author: Mateusz Guzik <mjg@FreeBSD.org> Date: Fri Apr 21 18:06:13 2023 +0000 vfs: list enums on separate lines Requested by: kib
Using the C23 underlying enum type extension is IMO better and safer. Support with clang (since version 8) and GCC 13+: https://godbolt.org/z/n4EshMY1j
This is not a strict equivalent of what I proposed here (my stuff resizes as needed), but would also to the job and I don't care past that how it looks like.
The goal is to let v_type and v_state by byte-sized enums without being bitfields.
Feel free to commandeer the review or post your own, in which case I'll abandon this one
i also note "byte num" is a bad name, how do you call a variant which takes 2? :)
Or, for the sake of this commit, add atomic_load_enum_uint8
|935 ↗||(On Diff #118865)|
The general intent with sys/cdefs.h is to provide common facilities to adjust to compiler quirks, for any compilation environment. Also, you explicitly use types (uint8_t) provided by cdefs.h conumers, while sys/cdefs.h is supposed to be self-contained: it is the root of all header dependencies.
Put your hack into implementation namespace (prepend with two _) and find a better way to deal with type dependency.
|945 ↗||(On Diff #118865)|
Can you reformat these to have one symbol per-line?
|935 ↗||(On Diff #118865)|
sys/types.h indeed sound not bad, but I do not agree with the _KERNEL braces. The type name should be put into impl namespace, and be accessible to any consumer. Note that we have usermode that wants the struct vnode definition.
I think the second macro is unnecessary based on my experiments with godbolt. Otherwise LGTM once we've decided where to place it (and I'd suggest with the _KERNEL guard).
|938 ↗||(On Diff #118865)|
Plain "enum" appears to work fine with both.
|945–946 ↗||(On Diff #118865)|
|949 ↗||(On Diff #118865)|
I looks like this is unnecessary, GCC 9 only needs it on the enum def, uses will automatically be sized correctly:
Plain enum should be fine.
I do think that one line per symbol improves the code layout and makes future diffs smaller. Also, I think you could do
... VMARKER, VLASTTYPE = VMARKER, };
I'd prefer if the __enum_uint8(vtype) changes were only inside structs and not in the function signatures but I don't feel strongly about it.
If the definition is visible the compiler knows the size so plain enum does the right thing. If you prefer it this way to be explicit that's fine by me although I'd limit the change to just the struct definition, in the function signatures it's unnecessary IMO.
Can we respect the original name so that we can use enum name directly instead of __enum_uint8(name) ? Then the diff will be much smaller.
#define __enum_uint8_decl(name) enum (name) : uint8_t