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

2023-07-13 Thread Johnny Billquist




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

2023-07-13 Thread Taylor R Campbell
> 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 *) = &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-->  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
>