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 <thor...@me.com> > > > On Jul 11, 2023, at 3:17 AM, Taylor R Campbell <riastr...@netbsd.org> 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 <mo...@rodents-montreal.org> > > 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 <mo...@rodents-montreal.org> > > > 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 <b...@softjar.se> > > 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 <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 | 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;