Re: [PATCH v3 2/3] docs/devel/style: add a section about bitfield, and disallow them for packed structures

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 28/11/24 21:15, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   docs/devel/style.rst | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 2f68b500798..2d73e6a8f7a 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -416,6 +416,26 @@ definitions instead of typedefs in headers and function prototypes; this
>   avoids problems with duplicated typedefs and reduces the need to include
>   headers from other headers.
>   
> +Bitfields
> +---------
> +
> +C bitfields can be a cause of non-portability issues, especially under windows
> +where `MSVC has a different way to lay them out than gcc

"GCC" (as MSVC).

> +<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_, and on big and

"and on" sounds odd to me. Maybe ", or where endianness matters."?

> +little endian hosts.
> +
> +For this reason, we disallow usage of bitfields in packed structures and in any
> +structures which are supposed to exactly match a specific layout in guest
> +memory. Some existing code may use it, and we carefully ensured the layout was
> +the one expected.
> +
> +We also suggest avoiding bitfields even in structures where the exact
> +layout does not matter, unless you can show that they provide a significant
> +memory usage or usability benefit.

I don't think we should mention "significant memory usage benefit".

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +
> +We encourage the usage of ``include/hw/registerfields.h`` as a safe replacement
> +for bitfields.
> +
>   Reserved namespaces in C and POSIX
>   ----------------------------------
>