Re: [PATCH] style(5): No struct typedefs

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

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 <thorpej%me.com@localhost>
> 
> > On Jul 14, 2023, at 2:56 AM, Martin Husemann <martin%duskware.de@localhost> 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 <brook%nmsu.edu@localhost>
> 
> 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" <tmm%mcci.com@localhost>
> 
> 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 <riastradh%NetBSD.org@localhost>
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;