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

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

I'm revising the patch to say `avoid' rather than `do not use'.  Style
rules can always be broken if there's a sufficiently compelling reason
to do so.  (For example, pthread_t is not going to go away no matter
how much I convince NetBSD's /usr/share/misc/style to hop and scream
at it.)


> Date: Tue, 11 Jul 2023 05:56:27 -0700
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> > On Jul 11, 2023, at 3:17 AM, Taylor R Campbell <riastradh%NetBSD.org@localhost> wrote:
> > 
> > If we used `struct bus_dma_tag *' instead, the forward declaration
> > could be `struct bus_dma_tag;' instead of having to pull in all of
> > sys/bus.h, _and_ the C compiler would actually check types.
> 
> In the original design, it's not always a struct.  That was the
> whole point of using a more abstract type.

It doesn't have to be a struct.  It can be any object that can be
converted to and from struct bus_dma_tag *.  It could be a
NUL-terminated string, with the char * cast to struct bus_dma_tag *
inside the implementation.

Or, a machine with two DMA spaces could represent them by (struct
bus_dma_tag *)(uintptr_t)0 and (struct bus_dma_tag *)(uintptr_t)1.
The _interface_ is type-safe this way (even if the _implementation_
relies on casts in one place) and doesn't require a profusion of
#include <sys/bus.h> everywhere just to refer to the bus_dma_tag type.

> If you want to hide the struct'ness in a machdep header file, fine,
> but I completely disagree with the notion of requiring the use of
> the "struct" keyword all over the place.

I agree the keyword is ugly, and it's unfortunate that in order to
omit it we would have to use C++, but the ugliness gives us practical
benefits of better type-checking, reduced header file maintenance
burden, and reduced motivation for unnecessary header file
dependencies.

(Witness the current spate of build failures arising from a painful
tangle of header file dependencies that I'm still not done
disentangling -- and this is far from the first time I've solved
tangled header file problems by just nixing includes and replacing
them by forward struct declarations.  I would like to do the same for
kauth_cred_t, for example, which has been a persistent problem in my
experience, and this kind of thing is getting in the way of critically
important diagnostics that I'm trying to add to crash(8).)


> Date: Tue, 11 Jul 2023 08:56:56 -0400 (EDT)
> From: Mouse <mouse%Rodents-Montreal.ORG@localhost>
> 
> But most - all, I think - of the benefits you cite are still available
> when using typedefs for the structs themselves.  Indeed, different
> files do not have to agree on whether to use typedefs, and external
> references, such as your
> 
> > struct vnode;
> > int frobnitz(struct vnode *);
> 
> can do exactly that, even if other code does "typedef struct vnode
> VNODE;" and then uses VNODE (or vnode_t, or whatever name you prefer;
> personally, I like all-caps).

The other code has to know to write `typedef struct vnode vnode_t;'
every time, and not, for example, `typedef struct vnode *vnode_t;'.
Excuse me, I mean `typedef struct vnode_s VNODE;'.  Errrrr, `typedef
struct vnode VNODE;'.

With forward struct declarations, there is exactly one name involved,
and if you misspell it in one place, it won't match all the other
places in a way that a compiler will noisily report.

Which pairs of options I gave will have the same effect?  Is VNODE the
same as vnode_t?  Is the evening star the same as the morning star?
This requires more thought to figure out.

Better to not have to think about it or worry that it has gotten out
of sync.  Of course, there is another way to ensure it hasn't gotten
out of sync: put it in one place, a header file.  Which defeats the
purpose of avoiding extraneous header file dependencies.

If there's an extremely strong justification for a typedef, then it is
possible to reduce the fragility of header file dependencies by
splitting up (say) foo.h into foo_types.h, with just a typedef, and
foo.h, with everything else.  But this is rare, and takes extra work
that almost always happens after the problem has already wasted enough
development time to infuriate someone like me enough to go to the
trouble of splitting it up.  Better to adopt a practice that doesn't
bring that burden on anyone in the first place.

> > There isn't even any need to define `struct bus_dma_tag' anywhere;
> > the pointer can be cast in sys/arch/x86/x86/bus_dma.c to `struct
> > x86_bus_dma_tag', for instance (at some risk in type safety, but a
> > much smaller risk than having void * as the public interface),
> 
> But at a risk in formal nonportability, unless the struct bus_dma_tag *
> was obtained by casting _from_ a struct x86_bus_dma_tag * to begin with
> (which in this case it probably would have been).

That is exactly what I meant.  The object could be _used_, in the
sense of reading and writing its members in arch/x86/x86/bus_dma.c,
only as a struct x86_bus_dma_tag, but the pointer to it can be passed
around as opaque struct bus_dma_tag *.

>                                                    I'd have to look up
> the details to tell whether it's possible for casting a pointer to a
> completed struct type to a pointer to a never-completed struct type to
> lose information, fall afoul of alignment requirements, or the like.

C11, Ch. 6 `Language', Sec. 6.3 `Conversions', Subsec. 6.3.2 `Other
operands', subsubsec. 6.3.2.3 `Pointers', paragraph 7:

  `A pointer to an object type may be converted to a pointer to a
   different object type.  If the resulting pointer is not correctly
   aligned for the referenced type, the behavior is undefined.'

But an incomplete object type has no alignment requirements, so it
can't be incorrectly aligned.  Only complete object types have
alignment requirements:

C11, Ch. 6 `Language', Sec. 6.2 `Concepts', Subsec. 6.2.8 `Alignment
of objects', paragraph 1 (emphasis added):

  `_Complete_ object types have alignment requirements which place
   restrictions on the addresses at which objects of that type may be
   allocated.'


> Date: Tue, 11 Jul 2023 09:28:49 -0400 (EDT)
> From: Mouse <mouse%Rodents-Montreal.ORG@localhost>
> 
> > But defining something like
> 
> > typedef struct bus_dma_tag *bus_dma_tag_t;
> 
> > would mean we could easily change what bus_dma_tag_t actually is,
> > keeping it opaque, while at the same time keeping the type checking.
> 
> Um, no, you get the type checking only as long as "what [it] actually
> is" is a tagged type - a struct, union, or (I think; I'd have to check)
> enum.  Make it (for example) a char *, or an unsigned int, and you lose
> much of the typechecking.

It is always safe to convert a pointer to struct bus_dma_tag to a
pointer to char, and, if alignment requirements are met (which, if
struct bus_dma_tag is an incomplete type, are always met), vice versa.

But only with a cast, of course, and we can limit those casts to the
internals of arch/*/*/bus_dma.c or similar; without an explicit cast,
the C compiler will reject any implicit conversion here for
type-safety.

Similarly, as long as sizeof(unsigned int) <= sizeof(uintptr_t), which
is the case for all architectures NetBSD runs on, it is always safe to
convert an unsigned int (via uintptr_t) to a pointer to struct
bus_dma_tag, and vice versa.


> Date: Tue, 11 Jul 2023 15:43:59 +0200
> From: Johnny Billquist <bqt%softjar.se@localhost>
> 
> Maybe I missed your point. Yes, if you typedef something based on some 
> simple type like int, that it's no different than any other int.
> 
> typedefs in C don't really create new types. They are all just 
> derivatives.

Yes, the story might be different if `typedef' or something created a
distinct type like `newtype' in Haskell or `type' in Go, on which you
could hang extra semantics at compile-time.

From f22bc5dd5ef79450cb5cab62976ff0f2a7553943 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 | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/share/misc/style b/share/misc/style
index 1c02dccecf2a..3520f919023e 100644
--- a/share/misc/style
+++ b/share/misc/style
@@ -130,6 +130,13 @@ int frobnicate(const char *);
 /* Then, there's a blank line, and the user include files. */
 #include "pathnames.h"		/* Local includes in double quotes. */
 
+/*
+ * Forward declarations for struct (and union) tags that don't need
+ * definitions go next.
+ */
+struct dirent;
+struct statfs;
+
 /*
  * Declarations for file-static functions go at the top of the file.
  * Don't associate a name with the parameter types.  I.e. use:
@@ -207,10 +214,20 @@ struct foo {
 };
 struct foo *foohead;		/* Head of global foo list */
 
-/* Make the structure name match the typedef. */
+/*
+ * Avoid creating typedefs for structs/unions or pointers to
+ * structs/unions.
+ *
+ * Using a typedef obscures whether the object is a struct/union or a
+ * pointer to a struct/union, and creates unnecessary header file
+ * dependencies when only pointers to them are passed around.  Instead,
+ * use forward declarations of struct/union tags in that case.
+ */
+#ifdef BAD
 typedef struct BAR {
 	int level;
 } BAR;
+#endif
 
 /* C99 uintN_t is preferred over u_intN_t. */
 uint32_t zero;