Updated patch: - Phrases advice more positively, less negatively.
- Clarifies that forward declarations go in .h files where it helps reduce header file dependency tangles, not .c files where it's seldom needed. - Gives some narrow exceptions to the guideline. > Date: Fri, 14 Jul 2023 05:11:48 -0700 > From: Jason Thorpe <thor...@me.com> > > > On Jul 14, 2023, at 2:56 AM, Martin Husemann <mar...@duskware.de> wrote: > > > > The typedef itself buys you nothing but a few charactes less to type, > > No, that's not correct. For a type that's meant to be "opaque" > (i.e. "users of the interface should not have to know or care about > any of the implementation details, historical leakage be damned"), > the typedef gives you true opacity... if you're using "struct kmutex > *" rather than "kmutex_t *" then you're already eating the forbidden > fruit. Right. There may be very rare cases where the leeway to define a type as a small integer on some ports, but a pointer or a larger struct on other ports, is appropriate, and the proposed style guideline doesn't prohibit doing that. (I don't think bus_dma_tag_t is one of those cases -- e.g., it has to be able to handle bus_dmatag_subregion from possibly hundreds of devices, so a uint8_t is not going to cut it -- but it's not that important; I'm not about to change it.) For the vast majority of kernel data structures that have pointers passed around or stored in data structures, however, the typedef exchanges a few characters of brevity for a gigantic pain in header file maintenance. > Date: Fri, 14 Jul 2023 08:15:36 -0600 > From: Brook Milligan <br...@nmsu.edu> > > I'm not sure I see the benefit of forward declarations being > scattered everywhere, which seems to be the focus of the proposed > guidelines. The benefit is reducing unnecessary header file dependencies: before after ------ ------ #include <sys/mutex.h> struct kmutex; /* not even needed here */ struct sched_percpustate { struct sched_percpustate { ... ... kmutex_t *spc_mutex; struct kmutex *spc_mutex; kmutex_t *spc_lwplock; struct kmutex *spc_lwplock; ... ... }; }; sys/mutex.h defines `typedef struct kmutex kmutex_t;' and defines the `struct kmutex' type. Users like sys/sched.h are satisfied by an incomplete type -- because they only involve pointers to the type -- and don't need the full `struct kmutex' definition, which drags in the tangle of header file dependencies. Such users don't even need a forward declaration, really, if they don't declare any function types involving `struct kmutex', and it's only because of the quirk in the C standard that even that requires a forward declaration. In the case of sys/sched.h, we could just omit `struct kmutex;' altogether (but not in sys/condvar.h, which declares functions like `void cv_wait(struct kcondvar *, struct kmutex *)' requiring a forward declaration). If you've been following source-changes and the releng autobuild dashboard over the past week, you'll see why I am extremely frustrated by unnecessary header file dependencies like this. If not, I encourage you to take a look at all the work that martin, mrg, and I (and possibly others I lost track of) have been doing all week to get the builds back up, with a litany of different failures across dozens of different ports, after what should have been a very small change to try to use sys/mutex.h from crash(8) for a critically important diagnostic we should have had 15 years ago. > Why is that preferable to having a (or multiple) header file(s) with > the forward declarations, e.g., foo_fwd.h, which would be included > by *.c implementation files that provide definitions (to ensure > consistency) and also by other files (*.h or *.c) that just need the > forward declarations to use opaque types. We could introduce a file sys/kmutex_fwd.h with the single line `struct kmutex;'. What would be the advantage of creating and maintaining an extra file over just writing the forward declaration? #include <sys/kmutex_fwd.h> versus struct kmutex; > This approach defines each identifier in one place, which leads to > consistency, and allows files to use either the opaque type (i.e., > forward declaration) or the concrete type (i.e., definition) as > needed. Forward declarations of incomplete struct types cannot become inconsistent. If the public interface involves `struct kmutex *', you can't write a wrong forward declaration `struct kmutex;' -- at worst, you might _omit_ a forward declaration and then use `struct kmutex *' in function parameter types, but the compiler will complain if that would cause trouble. > Date: Fri, 14 Jul 2023 10:24:34 -0400 > From: "Terry Moore" <t...@mcci.com> > > I've had to change internal implementations in our code from > "struct" to "union" at the top level. (This was because of a need to > increase the level of abstraction in the implementation.) > > That refactor had no impact on client code. Our code uses typedef > style, and whatever its merits, the style allows refactoring of the > implementation, provided that you separate the API header files from > the implementation (and don't disclose the implementation for use by > client source code). What did that accomplish that you couldn't accomplish with an anonymous union member? struct foo { union { float f; int i; }; }; What can you do by changing all users, or a typedef, to `union foo' that you can't do when the users/typedef are still `struct foo'? (Anonymous union members have been standard since C11, for over a decade now. Likewise anonymous struct members, which makes me realize there is a conciser alternative to writing `struct foo' everywhere -- you can use `union foo' with a single anonymous struct member, and it's one character shorter!)
>From 724e1b6101fd3d4397d34ac2678ec07aa2d3ba28 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell <riastr...@netbsd.org> Date: Tue, 11 Jul 2023 09:57:54 +0000 Subject: [PATCH] style(5): Advise against new struct typedefs and explain why. --- share/misc/style | 81 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 75 insertions(+), 6 deletions(-) diff --git a/share/misc/style b/share/misc/style index 1c02dccecf2a..84706319994c 100644 --- a/share/misc/style +++ b/share/misc/style @@ -57,6 +57,76 @@ __RCSID("$NetBSD: style,v 1.74 2023/04/21 16:12:53 rillig Exp $"); #ifndef _SYS_SOCKET_H_ #define _SYS_SOCKET_H_ +/* + * Include other header files only as necessary, mainly for type + * definitions or macros that are necessary to use in this header file. + * + * Avoid relying on transitive inclusions. + * + * Avoid header files dependencies just for struct and union types that + * are used in pointer types, which don't require type defintions. + * Instead, use forward declarations of the struct or union tag. + */ +#include <sys/foobar.h> + +/* + * Forward declarations for struct and union tags that don't need + * definitions go next. + */ +struct dirent; + +/* + * Define public structs and unions, only if they are user-allocated or + * otherwise exposed to users for a good reason; otherwise keep them + * private to .c files or `_impl.h' or `_private.h' files. + * + * Do not create a typedef like `typedef struct example example_t;' or + * `typedef struct example *example_t;'. Use `struct example' or + * `struct example *' in the public API; that way, other header files + * which declare functions or define struct or union types that involve + * only pointers to `struct example' need not pull in unnecessary + * header files. + */ +struct example { + struct data *p; + int x; + char y; +}; + +/* + * Use typedefs judiciously. + * + * Function or function pointer types: + */ +typedef void sighandler_t(int); + +/* + * Aliases for arithmetic types: + */ +typedef uint16_t nlink_t; + +/* + * Types that might be defined differently in some contexts, like + * uint8_t on one port, a pointer to a struct on another port, and an + * in-line struct larger than a pointer on a third port: + */ +typedef uint8_t foo_t; /* Hypothetical leg26 definition */ +typedef struct foo *foo_t; /* Hypothetical i786 definition */ +typedef struct { /* Hypothetical risc72 definition */ + uint32_t p; + uint32_t q; + uint8_t t; +} foo_t; + +/* + * For opaque data structures that are always represented by a pointer + * when stored in other data structures or passed to functions, don't + * use a type `foo_t' with `typedef void *foo_t'. Use `struct foo *' + * with no public definition for `struct foo', so the compiler can + * detect type errors, and other header files can use `struct foo *' + * without creating header file dependencies. + */ + /* * extern declarations must only appear in header files, not in .c * files, so the same declaration is used by the .c file defining it @@ -71,7 +141,7 @@ __RCSID("$NetBSD: style,v 1.74 2023/04/21 16:12:53 rillig Exp $"); */ extern int frotz; -int frobnicate(const char *); +int frobnicate(const char *, struct dirent *, foobar_t); /* * Contents of #include file go between the #ifndef and the #endif at the end. @@ -195,6 +265,10 @@ enum enumtype { * * It may be useful to use a meaningful prefix for each member name. * E.g, for ``struct softc'' the prefix could be ``sc_''. + * + * Don't create typedef aliases for struct or union types. That way, + * other header files can use pointer types to them without the header + * file defining the typedef. */ struct foo { struct foo *next; /* List of active foo */ @@ -207,11 +281,6 @@ struct foo { }; struct foo *foohead; /* Head of global foo list */ -/* Make the structure name match the typedef. */ -typedef struct BAR { - int level; -} BAR; - /* C99 uintN_t is preferred over u_intN_t. */ uint32_t zero;