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

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

> Date: Tue, 11 Jul 2023 19:50:19 -0700
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> > On Jul 11, 2023, at 2:56 PM, Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> 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
<sys/mutex.h>, 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 <kre%munnari.OZ.AU@localhost>
> 
> 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 *) = &foo;

    But you can do this:

    struct s;
    int foo(struct s *);
    int (*bar)(struct s *) = &foo;


> 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 <sys/mutex.h>   -->  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 ought be treated suspiciously

I tend to agree.  What part of the proposal makes you think this is
likely to happen in .c files?  I expect it to affect .h files by
obviating the need to include other .h files.

[everything else premised on putting these forward declarations in .c
files elided]


> Eg: (and deliberately using an absurd example, to avoid people
> trying to correct my misunderstandings of any real examples)

Maybe a real-world example would be more compelling -- but more to the
point, you're presenting a contrived counterexample as if I proposed a
theorem about all cases.  A general style rule can have exceptions;
what's important is the common cases, not the contrived
counterexamples.


> Date: Wed, 12 Jul 2023 13:40:27 -0400 (EDT)
> From: Mouse <mouse%Rodents-Montreal.ORG@localhost>
> 
> Hmm.  What value is provided by separating the vnode_t type from the
> rest of the vnode interface (in <sys/vnode.h>)?

It reduces header file dependencies, reducing incremental build costs,
reducing maintenance burden of header files, and reducing accidental
transitive inclusions.

The last week has been spent disentangling a nightmare of these
dependencies to get the build going again, after I added some very
useful functionality to crash(8) which meant exposing machine/mutex.h
to userland.  You can see a snapshot of the difficulty here:

https://web.archive.org/web/20230713220210/https://releng.netbsd.org/cgi-bin/builds.cgi

But there's a simpler approach which doesn't require making decisions
about these boundaries.  The approach is to use `struct vnode *'
instead of `vnode_t *', which completely dispenses with the question
of what header file it has to come from because it doesn't have to
come from a header file at all.

Sometimes it is worthwhile to make decisions about how to split up
header files.  But the proposed rule obviates the need to make those
decisions in many common cases of passing around pointers to objects
that are opaque to most or all users.