[Swan-dev] style discussion: structure of .c files

[Andrew Cagney]

Are you talking about a program or a library?

In general terms:

- monolithic headers end up being a dumping ground for all sorts of
junk (what libreswan.h sucks in is scary!)

- (I argue) their indirect dependencies make long term maintenance
hard; its easier to eliminate direct dependencies

For programs:

- the large program pluto already has a required header - "defs.h"

- all the other programs are small (single .c files?) so really have
no need for a monolithic header - they can instead explicitly include
their direct dependencies

For libraries:

- is a monolithic header even helpful?  shunk.c, for instance, has no
reason to include anything beyond:

  #include <string.h>
  #include "shunk.h"

- lswlog_realtime.c, is similar, either it requires:

  #include <stdbool.h>    /* for bool type used in code */
  #include "realtime.h"
  #include "lswlog.h"

or even ("realtime.h" should have included <stdbool.h>) just:

  #include "realtime.h"
  #include "lswlog.h"

"libreswan.h" is the last thing it needs - the file doesn't even use
TRUE/FALSE or true/false! - yet now it sucks it in.

So rather than trying to inflict a monolithic mess on libraries, try
the following guidelines:

-- include headers should be wrapped in #ifndef HEADER_H et.al.
wrapper (not __HEADER as a leading underscore is reserved by C?)

-- include headers should directly include their direct dependencies;
and per common and existing convention, first the system includes
using <> and the local includes using ""

For instance, a header using "monotime_t" as a value parameter,
structure field, or return type, should directly include "monotime.h";
ditto for "size_t" and <stddef.h>, 'bool' and <stdbool.h>, and
'uint32_t' and <stdint.h>, ...

However, a struct reference isn't a direct dependency.  For instance
'struct lswlog *', just requires the declaration 'struct lswlog;'.

-- for new pass-by-value objects, try to declare them as 'typedef
struct [<object>?] { ..., } <object>_t and put them in "object".h

It makes it easier to find; and easier to directly include.  I moved
chunk_t to its own header because I could never find it; and an
unexpected but good consequence is that it allowed the removal of some
redundant includes.

-- library .c files should only need to include their direct
dependencies, and per-convention system headers then local headers ...

Finally, these rules aren't a "must".  Instead, when something breaks,
or is being re-organized, just use them as a guideline for how to
clean up the existing code.

Andrew



On 22 April 2018 at 17:40, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
> This is meant as a conversation starter.  Thoughts?
>
> 0. First line: a succinct description of the file
>
> 1. License bumpf
>
> 2. any things mandated by standards to be first
>
>    Unlikely example:
>         #define _POSIX_C_SOURCE 199506L
>    See
>         <https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html>
>
>    Real example:
>         #include <pthread.h>
>
> 3. #includes for headers supplied by the system.
>    Don't include system headers that are included by baseline header.
>    Generally these can  be in any order so put them in a nice order.
>    Some OS-specific headers have ordering constraints.
>    By including these system headers before our project
>    headers, we reduce the chance of interference
>
> 4. #include the appropriate baseline header
>
> 5. #include any additional project headers
>
> 6. the rest of the file in a nice order
>
> An exception can always be made where it makes things simpler or clearer.
>
> Examples: A chunk of code is conditionally compiled, and a header file
> is only needed for that chunk of code, it makes sense to put the
> #include within that conditional.
>
>
> What should be in the baseline header?  Definitions that are pervasive
> through the code for which the baseline applies.
> - stddef.h, stdint.h, inttypes.h, stdbool.h, stdlib.h?, string.h,
>   limits.h?
> - IETF-defined constants
> - project-pervasive declarations (some of our library routines)
>
> There may well have to be different baseline headers for different
> contexts:
>
> - kernel code and code that interfaces with it (they need to share
>   definitions)
>         linux/include/libreswan.h?
>
> - userland library code (i.e. the portable code that Henry design).
>   It is designed to not depend on anything about libreswan.
>
> - userland library code that knows something about libreswan but isn't
>   only used inside pluto.  This is tricky because it shares some
>   interfaces (logging) but not all with pluto.  Several different
>   programs can use it
>
> - pluto code
>
> - userland code that has a close relationship with pluto (whack)
>
> - userland non-pluto programs
>         include/lswtools.h?  Maybe not.
>
> Some existing project headers that perhaps should be available pervasively:
>         linux/include/libreswan.h?
>         include/constants.h
>         include/puluto-constands.h
>         include/chunk.h
>         include/shunk.h
>         include/deltatime.h
>         include/monotime.h
>         include/realtime.h
>         include/lswalloc.h
>
> Related conventions:
>
> For system headers: #include <>
> For project headers: #inlcude ""
>
> As much as is reasonable, every .c file that exports some definitions
> must have a corresponding .h file (with the same basename) that
> declares those definitions.
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev