Re: [PATCH] style(5): No struct typedefs
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
>
> > On Jul 14, 2023, at 2:56 AM, Martin Husemann 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
>
> 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 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
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"
>
> 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
RE: [PATCH] style(5): No struct typedefs
> On Fri 2023-07-14 08:12, Jason Thorpe 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. Jason is right about this. 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). --Terry
Re: [PATCH] style(5): No struct typedefs
> On Jul 14, 2023, at 6:11 AM, Jason Thorpe wrote: > >> On Jul 14, 2023, at 2:56 AM, Martin Husemann 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. I’m not sure I see the benefit of forward declarations being scattered everywhere, which seems to be the focus of the proposed guidelines. 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. 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. Perhaps I am not understanding the point of the proposal or am underestimating the problem of files that provide forward declarations to be used elsewhere. The latter are unlikely to change rarely though (i.e., only when types are redefined in ways that affect the forward declaration), so the physical dependencies will not translate into recompilations in practice. Cheers, Brook
Re: [PATCH] style(5): No struct typedefs
> On Jul 14, 2023, at 2:56 AM, Martin Husemann 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. -- thorpej
Re: [PATCH] style(5): No struct typedefs
On Fri, Jul 14, 2023 at 12:36:30PM +0200, Johnny Billquist wrote: > It also brings that if you want to change the definition, you change it in > one place, and not 1000. Only if you change the name of the struct or decide it is not going to be a struct any more. All that is to be duplicated (from Taylor's example upthread) is: struct kmutex; > But I see that your desire is to not have a common place for definitions of > shared things. That is certainly a choice as well. Common things should only be defined once. There should not be more than one declaration of struct kmutex and its members. But multiple forward declarations (if needed) are fine. Martin
Re: [PATCH] style(5): No struct typedefs
On 2023-07-14 11:56, Martin Husemann wrote: On Fri, Jul 14, 2023 at 01:00:26AM +0200, Johnny Billquist wrote: Using "typedef struct bus_dma_tag *" instead of "typedef void *" will do the same thing. That is no reason at all why to skip the typedef. We want to avoid having to #include the header where that typedef lives. The typedef itself buys you nothing but a few charactes less to type, but it introduces another layer of indirection where you could (accidently) introduce inconsistencies (unless you include it from some common header, which brings us back to the start). It also brings that if you want to change the definition, you change it in one place, and not 1000. But I see that your desire is to not have a common place for definitions of shared things. That is certainly a choice as well. Johnny -- Johnny Billquist || "I'm on a bus || on a psychedelic trip email: b...@softjar.se || Reading murder books pdp is alive! || tryin' to stay hip" - B. Idol
Re: [PATCH] style(5): No struct typedefs
On Fri, Jul 14, 2023 at 01:00:26AM +0200, Johnny Billquist wrote: > Using "typedef struct bus_dma_tag *" instead of "typedef void *" will do the > same thing. That is no reason at all why to skip the typedef. We want to avoid having to #include the header where that typedef lives. The typedef itself buys you nothing but a few charactes less to type, but it introduces another layer of indirection where you could (accidently) introduce inconsistencies (unless you include it from some common header, which brings us back to the start). Martin
Re: [PATCH] style(5): No struct typedefs
On 2023-07-14 00:22, Taylor R Campbell wrote: Date: Tue, 11 Jul 2023 19:50:19 -0700 From: Jason Thorpe On Jul 11, 2023, at 2:56 PM, Taylor R Campbell wrote: 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. No -- you just don't have to use "void *". Can you point to a practical problematic example? Using `struct bus_dma_tag *' instead of `void *' (whether via the bus_dma_tag_t alias or not) would provide better type-checking. Using "typedef struct bus_dma_tag *" instead of "typedef void *" will do the same thing. That is no reason at all why to skip the typedef. And I totally agree that void * is usually something to be avoided, if possible. But I still fail to see what it has to do with the topic on typedef or not. Johnny -- Johnny Billquist || "I'm on a bus || on a psychedelic trip email: b...@softjar.se || Reading murder books pdp is alive! || tryin' to stay hip" - B. Idol
Re: [PATCH] style(5): No struct typedefs
> Date: Tue, 11 Jul 2023 19:50:19 -0700 > From: Jason Thorpe > > > On Jul 11, 2023, at 2:56 PM, Taylor R Campbell > > wrote: > > > > 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. > > No -- you just don't have to use "void *". Can you point to a > practical problematic example? Using `struct bus_dma_tag *' instead of `void *' (whether via the bus_dma_tag_t alias or not) would provide better type-checking. Exposing `struct bus_dma_tag *' instead of bus_dma_tag_t as part of the API would reduce header file maintenance burden and unnecessary header file dependencies. But you folks are right that bus_dma_tag_t was confusing, because there are two different issues behind it. kmutex_t is a better example of the header file dependency issue, because a lot of the kernel just passes around pointers to struct kmutex and doesn't need the definition, like sys/condvar.h. This can be served by `struct kmutex *', which doesn't require #include , instead of `kmutex_t *', which does. The `struct kmutex' definition is particularly difficult, but since callers statically allocate them, must be exposed. In other cases, like pserialize_t, the definition can be completely opaque. A header file that just needs to store a pserialize_t or percpu_t reference would have no need to pull in sys/pserialize.h or sys/percpu.h if we just used `struct pserialize *' or `struct percpu *' instead. (Quiz: Without looking, between pserialize_t and percpu_t, which one is a pointer type and which one is a struct type you have to use with *, or is it both or neither?) > Date: Wed, 12 Jul 2023 11:51:41 +0700 (ICT) > From: Robert Elz > > But I 100% disagree with the notion of declaring those opaque > struct types over and over again in each individual source file. > Declarations should appear exactly once - either in a source flle > if the object concerned is used only in that file. Otherwise in > a header file. Aside from a quirk of C scope rules for tags[*], there is no semantic content to the `struct s;' forward declaration, so it's harmless to repeat it -- if `struct s' figures into a public part of the API, the `struct s' aspect of that can't get out of sync no matter how many times you write a `struct s' forward declaration. Were it not for that quirk, it wouldn't even be necessary to have the forward declaration in the first place! In contrast, one compilation unit could have `typedef struct s s_t;' while another has `typedef struct s *s_t;' and a third has `typedef void *s_t;'. If the public part of the API is `s_t' instead of `struct s', these could get out of sync. That's why it's necessary for `s_t' to have a single definition in a header file. But if the public part of the API is to use `struct s' instead of `s_t', that can't be a problem. So it is safe for function declarations that pass around pointers to `struct s', or for struct definitions with pointers to `struct s' as members, to skip the header file. [*] In principle, there wouldn't even really be a need for these forward declarations, but for a peculiar clause of C scoping for tags (C11, Ch. 6 `Language', Sec. 6.2 `Concepts', Subsec. 6.2.1 `Scopes of identifiers', paragraph 4, pp. 35-35): If the declarator or type specifier that declares the identifier appears within the list of parameter declarations in a function prototype (not part of a function definition), the identifier has function prototype scope, which terminates at the end of the function declarator. As a consequence, you can't do something like this, because the two `struct s' tags are distinct types: int foo(struct s *); int (*bar)(struct s *) = But you can do this: struct s; int foo(struct s *); int (*bar)(struct s *) = > I'm not even sure it is practically possible (in many cases anyway) > to do it as you're suggesting, as to use an opaque struct that way, > you're almost certainly using it as the parameter of one of more > public functions in your source file, in which case those need to > be declared in a header file, and so that neader file also needs > the opaque struct definition. Here's an example where I fixed the build by doing nothing but replacing #include--> struct kmutex; kmutex_t *...; --> struct kmutex *...; (and fixing some downstream accidents of transitive inclusion): https://mail-index.netbsd.org/source-changes/2023/07/13/msg145972.html This is far from the first time I've solved problems just by making this substitution. The style rule would obviate the need for most build fixes like this because it would be the default way to do things. > I'm tempted to say that an opaque struct declaration in a .c > file
Re: [PATCH] style(5): No struct typedefs
>> [...] as I see it the divide you're sketching here ([...]) is the >> divide between interface and implementation, and in some cases the >> interface is more than just the typedefs. > Sort of. > // contains the "vnode_t" opaque type definition > // contains the guts of "struct vnode" and the other > implementation details > // Contains some of the file system interfaces, some of which > use vnode_t > // Contains the vnode interfaces, which definitely use vnode_t > The latter if the two would each include . You're right, I hadn't fully understood you. Hmm. What value is provided by separating the vnode_t type from the rest of the vnode interface (in )? If taken to its logical extreme (which of course ends up at a satirical position, like most logical extremes), this leads to // vnode_t // enum vtype // enum vtagtype // #define VNODE_TAGS // struct vnlock // #define IO_UNIT // #define IO_APPEND which I hope you agree is madness. What makes it worth singling out vnode_t for special treatment here? I would prefer to draw include-file boundaries more or less matching conceptual-API boundaries, which I thought was more or less where we started: defines the API to the vnode subsystem, including types, #defines, externs, etc. But I'm not sure how that would differ from what we have now. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: [PATCH] style(5): No struct typedefs
> On Jul 12, 2023, at 5:25 AM, Mouse wrote: > > [paragraph-length-line damage repaired manually] > >> What about something like and >> ? The type definitions go into the former >> header file, [...] > > Well, I don't like the "typedefs" name, Yah, I don't like it either, but.. :-) > because as I see it the divide > you're sketching here (which I support, in general) is the divide > between interface and implementation, and in some cases the interface > is more than just the typedefs. Sort of. // contains the "vnode_t" opaque type definition // contains the guts of "struct vnode" and the other implementation details // Contains some of the file system interfaces, some of which use vnode_t // Contains the vnode interfaces, which definitely use vnode_t The latter if the two would each include . Feel free to pick a better name, but that's the idea. -- thorpej
Re: [PATCH] style(5): No struct typedefs
> I'm tempted to say that an opaque struct declaration in a .c file
> ought be treated suspiciously -
Depending on what you mean by "an opaque struct declaration", I may
agree or disagree.
If you mean the "struct foo;" without a body, then I think I agree.
But the "struct foo { ... };" that completes the incomplete type
corresponding to the include file's "struct foo;", that I think is the
whole point of opaque structs: the completion is available only under
the implementation hood. While that may be in a foo-impl.h file, if
only a single file needs it I see no harm in the completion being in
the .c (and indeed I've done it that way often enough).
> (and it would be kind of nice if C had a way to say "all functions
> defined beyond this point are static").
Personally, I'd prefer "all functions defined before this point are
static", since I prefer calls to move textually backward (or, to put it
another way, I prefer to avoid forward declarations when possible).
But I doubt either of those will appear anytime soon.
> And to return briefly, to the issue way up the top of simplifying the
> leader files, there is one change I've wanted to make for ages, but
> just haven't been brave enough to do,
> That is to rip the definition of __NetBSD_Version__ [...] into a new
> header file all of its own [...] with the rule that *no* other header
> file is oermitted to include it.
I'm...not sure I agree with that.
I once built a kernel facility which I wanted to be completely drop-in
to multiple kernel versions (as widely disparate as 1.4T and 5.2). The
design I came up with was (names probably changed; I'm not digging up
the code to see what names I actually used) a "version.h" file which
looked like
#include // to get __NetBSD_Version__
#if __NetBSD_Version__ == whatever value 1.4T had
#include // where 1.4T keeps it
#define THINGY() do { ...1.4T code for THINGY() ... } while (0)
typedef int COMMON_TYPE; // just an int on 1.4T
#endif
#if __NetBSD_Version__ == whatever value 5.2 had
#include // on 5.2 we need two
#include // include files
#define THINGY() thingy() // 5.2 has THINGY() nicely encapsulated
typedef struct something COMMON_TYPE; // 5.2 uses a struct
#endif
etc. It sounds as though you would prefer the #include that pulls in
__NetBSD_Version__ be in every C file that wants to include
"version.h". This seems counterintuitive, even counterproductive, to
me (and see also below, about #include order). Or perhaps you'd prefer
that I'd designed those interfaces some other way, rather than with a
version-switch include file?
My own pet include-file peeve is rather different: I strongly believe
that, with very few exceptions ( is the only one that comes
to mind), you should be able to re-order your #include lines
arbitrarily without producing any semantic change, and that, if this is
not so, at least one of the include files involved is broken. I've
been making small steps towards fixing this in my own trees, but it's
still a major mess.
/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTMLmo...@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: [PATCH] style(5): No struct typedefs
[paragraph-length-line damage repaired manually] > What about something like and > ? The type definitions go into the former > header file, [...] Well, I don't like the "typedefs" name, because as I see it the divide you're sketching here (which I support, in general) is the divide between interface and implementation, and in some cases the interface is more than just the typedefs. Some structs have their struct definition, or some of it (regex_t is an example), as part of their advertised interface, and many have #defines as well. But I'd rather see the division done with (what I see as) an inaccurate name than see the division not done. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: [PATCH] style(5): No struct typedefs
> On Jul 11, 2023, at 9:51 PM, Robert Elz wrote: > > That is to rip the definition of __NetBSD_Version__ (with however > nany underscores it really has) out of and into a new > header file all of its own (perhaps - no bikeshedding > about names here please) with the rule that *no* other header file is > oermitted to include it. C code that needs __NetBSD_Version__ needs > to explcitly include that file itself ... there is surprisingly little > of it. This will make the side effects of doing a kernel version bump > be so small (really, so it should be) that doing one is an automatic > decision any time it might be needed, and we end the "even though this > is a kernel internal ABI change, I don't think any modules will be affected" > (and no no bump happens - which must be what happens sometimes, either that > of some of our developers don't understand what a kernal internal ABI > change means) and should also avoid the need for "ride the kernel > version bump" (perhaps several hours later) or "let me know when you're > going to bump the kernel version, I have changes to commit which would > need that, but don't need to be commmitted right away" - which often > turns into a "ride the bump" when the developer who did a change, and > a bump, did not remeber, or perhaps even know, the other developer was > waiting and would have likes to coordinate. I’ve been thinking about this a little more… What about something like and ? The type definitions go into the former header file, that can be included by whatever other header needs that type definition. The full-blown structure and internal details of it go into the latter, to be only included by things that access the guts, and now it’s VERY clear that “I am using implementation details” aspect of touching those guts. -- thorpej
Re: [PATCH] style(5): No struct typedefs
I agree with some of what you are proposing, but disagree with much of it. Certainly simplifying our header file mess is important, but that's not going to happen overnight. One particular example of that below. And using opaque etruct definitions where possible, rather than void * in particular is certainly the right thing to do - I suspect that the reason that we don't have nore of that now, is just based upon the age of the original NetBSD code, compared to when opaque structs became available - and then the tendency to copy existing code styles, rather than "breaking tradition". But I 100% disagree with the notion of declaring those opaque struct types over and over again in each individual source file. Declarations should appear exactly once - either in a source flle if the object concerned is used only in that file. Otherwise in a header file. By all means in a header file which doesn't do much else - we used to avoid such things, as the cost of opening and reading (over ane over again) lots of tiny header files was getting to be a much too large fraction of the overall compilation time, but compilers have gotten a lot smarter at doing that, and a lot slower at doing other parts of the compilation, so this shouldn't really be an issue any more. I'm not even sure it is practically possible (in many cases anyway) to do it as you're suggesting, as to use an opaque struct that way, you're almost certainly using it as the parameter of one of more public functions in your source file, in which case those need to be declared in a header file, and so that neader file also needs the opaque struct definition. Then all that is needed (which is needed anyway) is to include that header file. Ideally that header does little else. I'm tempted to say that an opaque struct declaration in a .c file ought be treated suspiciously - I thought there might be one use, where a file is providing a public interface using an opaque struct pointer, and then lower down the file, the implemenattion of those public functions using static functions (so no access is possible except via the public functions) with the complete struct definition occring between the two halves of the code (and it would be kind of nice if C had a way to say "all functions defined beyond this point are static"). But even that does not need, or want, an opaque declaration for a struct in the .c file, as that needs to be in the header file which declares the public functions anyway, and that needs to be included in the ..c file for type checking, even if not otherwise needed for anyhing useful. So I'd be tempted to have the style guide exokicitly say not to use opaque struct declarations in .c files - with the caveat that, as with all there, it is a guide, not a law, and when appropriate, it can be ignored. I also disagree on typedefs to structs, and while I don't particularly like them much myself, even typedfs to pointers to structs. Once you disabuse yourself of the idea that you can avoid using header files by redeclaring opaque structs in .c files all over the place, your argument against typedefs essentially avaporates - as it was, as I understood it, largely that using a typedef requires using a header file (true) - but since we are going to want that anyway, might just as well have it say struct foobar; typedef struct fobar foo; or typedef struct foobar *foo_ptr; (or both as appropriate). One obvious reason for using typedefs in this way is when we have a common object interface which is implemented entirely differently on different architectures. Eg: (and deliberately using an absurd example, to avoid people trying to correct my misunderstandings of any real examples) we might have a an an implementation defined type "dogleash". On some architectures all the proberties of a dogleash, except its length (which is expressed in mm, and no greater than 10m, is 1 mm) so a dogleasy type is an int (all that is requied). Another architexture is much more flexible, and requires the materials (leather, chain, woven plastic fibre, ...) the colour, the length, and the handle and attachment types to all be specified - and so clearly a dogleash os going to be a struct there. Then by your proposed quidelines, since typedefs for ints are permitted, but typedefs for structs are not, we'd end up with one of the following two abominations all over the place (in the MI code) #ifdef SIMPLE_LEASH void windup(dogleash); #else void windup(struct dogleash); #endif or #ifdef SIMPLE_LEASH #define leashtype #else #define leashtype struct #endif void windup(leashtype dogleash); and while the second of those is more pleasant to look at, in isolation, if actually done that way it would quickly become a nightmare to maintain - particularly if the most commonly used implementation architectures (the ones people mostly code and develop using) are the SIMPLE_LEASH type, where leaving out the "leashtype" word changes nothing
Re: [PATCH] style(5): No struct typedefs
> On Jul 11, 2023, at 2:56 PM, Taylor R Campbell > wrote: > > 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. No -- you just don't have to use "void *". Can you point to a practical problematic example? -- thorpej
Re: [PATCH] style(5): No struct typedefs
On Tue, 11 Jul 2023 at 06:17, Taylor R Campbell wrote: > > I propose the attached change to KNF style(5) to advise against > typedefs for structs or unions, or pointers to them. Perhaps be positive? Add a section encouraging the use of struct, union and enum forward declarations when the details aren't needed by the header? - I mention enums as compilers are finally getting good at checking them. - I've found that, over time, the number of unexplainable #includes does go down; but only when there's a willingness to go in and rip out the #include spaghetti in .c files > Passing around pointers to structs and unions doesn't require the > definition, only a forward declaration: > > struct vnode; > int frobnitz(struct vnode *); Be clear about where the forward declaration should go. Once, near the top after the required #includes, or inline. Also keep in mind that, just like #includes, these seem to accumulate over time; especially when inline. > This can dramatically cut down on header file dependencies so that > touching one header file doesn't cause the entire kernel to rebuild. > This also makes it clearer whether you are passing around a copy of a > struct or a pointer to a struct, which is often semantically important > not to conceal. > > Typedefs are not necessary for opaque cookies to have labels, like > `typedef void *bus_dma_tag_t;'. In fact, using void * here is bad > because it prevents the C compiler from checking types; bus_dma_tag_t > is indistinguishable from audio_dai_tag_t, and from pckbc_tag_t, and > from acpipmtimer_t, and from sdmmc_chipset_handle_t. > > 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. 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), and it > doesn't even violate strict aliasing rules. If a generic header were to forward declare `struct bus_dma_tag;` as part of an abstract interface, is each implementation allowed to provide their own local definition? It is valid C. The result can be a good thing, or a dangerous thing. Be clear either way. (constructs such as <> were often frowned upon because the code was crap; that's no longer true; doesn't make them a good idea though) I can see your point about typedef, void*, and hiding pointers, but there are always exceptions. For instance say the object in question is an opaque singleton. This week it is <> but next week it is <>. > (Typedefs for integer, function, and function pointer types are not > covered by this advice.) > > Objections? Just get a good lock.
Re: [PATCH] style(5): No struct typedefs
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 > > > On Jul 11, 2023, at 3:17 AM, Taylor R Campbell 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 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 > > 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;'. Er, `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
Re: [PATCH] style(5): No struct typedefs
> On Jul 11, 2023, at 6:43 AM, Johnny Billquist wrote: > > typedefs in C don't really create new types. They are all just derivatives. > Sometimes I even wonder why typedef exists in C. Feels like I could > accomplish the same with a #define Because #define generates pollution in unrelated things. -- thorpej
Re: [PATCH] style(5): No struct typedefs
On 2023-07-11 15:28, Mouse wrote: I don't get it. Why the "void *" stuff? That is where I think the real badness lies, and I agree we should not have that. 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. 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. Sometimes I even wonder why typedef exists in C. Feels like I could accomplish the same with a #define Johnny -- Johnny Billquist || "I'm on a bus || on a psychedelic trip email: b...@softjar.se || Reading murder books pdp is alive! || tryin' to stay hip" - B. Idol
Re: [PATCH] style(5): No struct typedefs
On 2023-07-11 15:28, Mouse wrote:
I don't get it. Why the "void *" stuff? That is where I think the
real badness lies, and I agree we should not have that.
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.
Sure. If you declare something as derived from char *, then anything
that expects something char * would be happy with it.
But if you say
typedef struct bus_dma_tag *bus_dma_tag_t;
then any function that expects this will not be happy with something
that translates to char *.
Basically, you get as much type checking as you could ever
expect/require/demand.
But I could agree with your point of not hiding the pointer in the
typedef, and have it explicit as well, for some situations. But for
something this opaque, I would actually think the pointer in there makes
sense.
Example:
---
typedef struct dma_bus_tag *dma_bus_tag_t;
typedef struct dma_bus2_tag *dma_bus2_tag_t;
int test(dma_bus_tag_t foo)
{
return 0;
}
int foo(void)
{
dma_bus2_tag_t x;
return test(x);
}
---
This fails at compilation because x is not of the correct type for the
function test(). If you change x to be of type dma_bus_tag_t, then the
compilation is happy. Typechecking just as you would expect it, while
being totally unaware of what this type actually looks like.
Something that actually uses the value obviously needs to have the full
definition of the structure, and dereference it, and so on. But all
other code do not need any of that, and can be kept totally in the dark.
Johnny
--
Johnny Billquist || "I'm on a bus
|| on a psychedelic trip
email: b...@softjar.se || Reading murder books
pdp is alive! || tryin' to stay hip" - B. Idol
Re: [PATCH] style(5): No struct typedefs
> Sometimes I even wonder why typedef exists in C. Feels like I could > accomplish the same with a #define For `typedef struct foo Something', yes, you could (well, except for their different scopes, and possibly some other corner cases). For `typedef void (*callback_t)(void *, status_t, int)', not so much. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: [PATCH] style(5): No struct typedefs
On Tue, Jul 11, 2023 at 05:56:27 -0700, Jason Thorpe wrote: > > On Jul 11, 2023, at 3:17 AM, Taylor R Campbell 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. The bus_dma_tag_t example from the original email is not the best one, but I didn't want to open that can of worms in my reply, so I mentioned the "not always struct" case without actually mentioning names. The style(5) specifically gives an example of a struct typedef, not of an opaque typedef. > 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 used to lean both ways at different times and in different contexts. I think that existence and usefulness of opaque typedefs is exactly the strong argument against using "convenience struct typedefs", b/c the latter dilute the message so to speak. If someone wants to program with "systems hungarian", they know where to find it... -uwe
Re: [PATCH] style(5): No struct typedefs
> I don't get it. Why the "void *" stuff? That is where I think the > real badness lies, and I agree we should not have that. > 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. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: [PATCH] style(5): No struct typedefs
On 2023-07-11 12:17, Taylor R Campbell wrote: I propose the attached change to KNF style(5) to advise against typedefs for structs or unions, or pointers to them. Passing around pointers to structs and unions doesn't require the definition, only a forward declaration: struct vnode; int frobnitz(struct vnode *); Agreed. But this is unrelated to typedefs. This can dramatically cut down on header file dependencies so that touching one header file doesn't cause the entire kernel to rebuild. This also makes it clearer whether you are passing around a copy of a struct or a pointer to a struct, which is often semantically important not to conceal. Typedefs are not necessary for opaque cookies to have labels, like `typedef void *bus_dma_tag_t;'. In fact, using void * here is bad because it prevents the C compiler from checking types; bus_dma_tag_t is indistinguishable from audio_dai_tag_t, and from pckbc_tag_t, and from acpipmtimer_t, and from sdmmc_chipset_handle_t. I don't get it. Why the "void *" stuff? That is where I think the real badness lies, and I agree we should not have that. 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. Basically, getting all the benefits you mention from having it as a proper type, but still also keeping the ability to change what it actually is without any problems. So, yes to proper forward declarations. But I don't think typedefs as such is a problem. Johnny -- Johnny Billquist || "I'm on a bus || on a psychedelic trip email: b...@softjar.se || Reading murder books pdp is alive! || tryin' to stay hip" - B. Idol
Re: [PATCH] style(5): No struct typedefs
[riastradh@] > I propose the attached change to KNF style(5) to advise against > typedefs for structs or unions, or pointers to them. Pointers to them, I agree. I don't like typedefs for pointers to structs, except possibly for a few special cases. I think it should be pellucid from the declaration whether you're dealing with a pointer. 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). > 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). 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. [uwe@] > Typedefs make sense when the type is *really* opaque and can, behind > the scenes, be an integer type, a pointer or a struct. Agreed. > [Ab]using typedefs to save 8 bytes of "struct " + "*" just adds > cognitive load (and whatever logistical complications that you have > enumerated in the elided part of the quote). Personally, I find that *in*cluding the "struct" adds cognitive load. Perhaps it's just a question of what I'm used to, but having a noise word present - and the "struct" is close to that, from a conceptual point of view - means more noise to ignore. Especially when the type is referred to multiple times; I haven't seen it often, but I have seen statements that look as though half the alphanumerics are "struct" (I doubt any actually make it to the point of half, since each "struct" needs a tag to be useful, and at least a few other identifiers to make a useful statement, but it sure feels like it occasionally). /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: [PATCH] style(5): No struct typedefs
> On Jul 11, 2023, at 3:17 AM, Taylor R Campbell 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. 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. -- thorpej
Re: [PATCH] style(5): No struct typedefs
Objections? No objections here, just concurrences. ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | ++--+--+
Re: [PATCH] style(5): No struct typedefs
On Tue, Jul 11, 2023 at 10:17:24 +, Taylor R Campbell wrote: > I propose the attached change to KNF style(5) to advise against > typedefs for structs or unions, or pointers to them. [...] > (Typedefs for integer, function, and function pointer types are not > covered by this advice.) Yes, please. Typedefs make sense when the type is *really* opaque and can, behind the scenes, be an integer type, a pointer or a struct. [Ab]using typedefs to save 8 bytes of "struct " + "*" just adds cognitive load (and whatever logistical complications that you have enumerated in the elided part of the quote). -uwe
Re: [PATCH] style(5): No struct typedefs
On Tue, Jul 11, 2023 at 10:17:24AM +, Taylor R Campbell wrote: > Objections? This is a very good change. Martin
[PATCH] style(5): No struct typedefs
I propose the attached change to KNF style(5) to advise against
typedefs for structs or unions, or pointers to them.
Passing around pointers to structs and unions doesn't require the
definition, only a forward declaration:
struct vnode;
int frobnitz(struct vnode *);
This can dramatically cut down on header file dependencies so that
touching one header file doesn't cause the entire kernel to rebuild.
This also makes it clearer whether you are passing around a copy of a
struct or a pointer to a struct, which is often semantically important
not to conceal.
Typedefs are not necessary for opaque cookies to have labels, like
`typedef void *bus_dma_tag_t;'. In fact, using void * here is bad
because it prevents the C compiler from checking types; bus_dma_tag_t
is indistinguishable from audio_dai_tag_t, and from pckbc_tag_t, and
from acpipmtimer_t, and from sdmmc_chipset_handle_t.
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. 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), and it
doesn't even violate strict aliasing rules.
(Typedefs for integer, function, and function pointer types are not
covered by this advice.)
Objections?
>From 69dc9658454c8edce695630d2e1bc75db86c43eb Mon Sep 17 00:00:00 2001
From: Taylor R Campbell
Date: Tue, 11 Jul 2023 09:57:54 +
Subject: [PATCH] style(5): Prohibit new struct typedefs and explain why.
---
share/misc/style | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/share/misc/style b/share/misc/style
index 1c02dccecf2a..85790024f3f0 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,17 @@ struct foo {
};
struct foo *foohead; /* Head of global foo list */
-/* Make the structure name match the typedef. */
+/*
+ * Do not create typedefs for structs or pointers to structs. Using a
+ * typedef obscures whether the object is a struct or a pointer to a
+ * struct, and creates unnecessary header file dependencies when only
+ * pointers to them are passed around.
+ */
+#ifdef BAD
typedef struct BAR {
int level;
} BAR;
+#endif
/* C99 uintN_t is preferred over u_intN_t. */
uint32_t zero;
