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: kibDetails
Details
- Reviewers
markj kib jrtc27 - Commits
- rGcebb8646c4ee: Support byte-sized enums
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| sys/sys/namei.h | ||
|---|---|---|
| 44 ↗ | (On Diff #118690) | ignore this bit |
Comment Actions
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
Comment Actions
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
Comment Actions
ping?
i also note "byte num" is a bad name, how do you call a variant which takes 2? :)
| sys/kern/vfs_cache.c | ||
|---|---|---|
| 3680 | i think it is time for atomic_load which figures out the size btw | |
| sys/kern/vfs_cache.c | ||
|---|---|---|
| 3680 | Or, for the sake of this commit, add atomic_load_enum_uint8 | |
| sys/sys/cdefs.h | ||
| 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) | |
| sys/sys/vnode.h | ||
| 59 | Can you reformat these to have one symbol per-line? | |
| sys/sys/cdefs.h | ||
|---|---|---|
| 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. |
Comment Actions
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).
| sys/sys/cdefs.h | ||
|---|---|---|
| 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: |
| sys/sys/vnode.h | ||
| 114 | Plain enum should be fine. | |
| sys/sys/vnode.h | ||
|---|---|---|
| 59 | 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, }; | |
Comment Actions
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.
| sys/sys/vnode.h | ||
|---|---|---|
| 114 | 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. | |
| sys/sys/types.h | ||
|---|---|---|
| 351 | 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 | |