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

[Robert Elz <kre%munnari.OZ.AU@localhost>]

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 10000 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
(as it is nothing) so will probably frequently happen, until
the other. less frequently used archs start getting build failures.


So, yes, please, opaque struct types where possible, but always defined
in a header file, not in the .c source files.   You may find less benefit
for avoiding void * from this than you're hoping though, as in many places
void * really is needed, as lots of wildly different pointer types pass
through that interface,

But no rules about typedefs, sometmes they're almost mandatory,
sometimes they're useful, sometmes they're annoying, and sometimes
it is really just a matter of the style of the developer.


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__  (with however
nany underscores it really has) out of <sys/param.h> and into a new
header file all of its own <sys/netbsd-version.h> (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.

As it is now, almost everything depends on <sys/param.h> (often
indirectly) which is mostly OK, as most of it almost never changes,
and if it does, large scale rebuilds are probably required, but
almost nothing actually cares about __NetBSD_Version__ - not even
the modules (except for getting them to be recompiled, just in case,
though the regular dependencies should handle that), and where they
get stored in the filesystem so the correct versions can be located.

Beyond that, there are also scripts (mostly build related) which
extract version info from <sys/param.h> - I've never see one of
other which is getting __NetBSD_Version__ and other info simultaneously
from paran,h, so in 99% of scripts like that, it should just be a
matter of changing the name of the file being operated upon (the format
of the __NetBSD_Version__ definition is made, including its comment,
would not change of course ... but if we have this as a standalons simple
file, we could also have a script which builds it, avoiding the occasional
case where a developer bumps the numeric value, but not the comment, or
vice versa, just running something kike "kerel-bump" would find the
include file in the current src tree, generate a replacement (just
as if editing it) and have it ready for a commit - no manual editing of
it required, ever again .. major bumps could be handled by options to
the script, or manually, as they tend to reqire mor changes elsewhere
anyway (macro files, www files, ...).

kre

ps: apologies if I do not respond quickly to replies, processing
e-mail is difficult for me for a while, until I get my PC fixed.