INN indentation

[Russ Allbery]

Julien ÉLIE <julien at trigofacile.com> writes:

> I've had a look but wanted to be sure of what to do.
> It seems like that all inclusions of "clibrary.h", or of both "clibrary.h"
> and "config.h", should be changed to only one inclusion of
> "portable/system.h".
> And "clibrary.h" moved to "portable/system.h".

I think that's right.

In my other packages, I leave in the include of config.h to be explicit
and because there are some files that don't need portable/system.h but
everything (pretty much) needs config.h.  But this is just me being
pedantic and it's probably not worth maintaining that in INN.

> "portable/system.h" in rra-c-util uses:
> #include <config.h>

> "clibrary.h" uses:
> #include "config.h"

> Shouldn't the syntax with quotes be used?  ("config.h" is not a system
> header)

My other packages use non-recursive make, and in that case I think it's
preferrable to not have the two different search paths for include files
in play (it tends to confuse me).  Automake explicitly adds the
appropriate header search path automatically.  The Autoconf manual
explicitly recommends this for config.h:

   To provide for VPATH builds, remember to pass the C compiler a ‘-I.’
   option (or ‘-I..’; whichever directory contains ‘config.h’).  Even if
   you use ‘#include "config.h"’, the preprocessor searches only the
   directory of the currently read file, i.e., the source directory, not
   the build directory.

   With the appropriate ‘-I’ option, you can use ‘#include <config.h>’.
   Actually, it’s a good habit to use it, because in the rare case when
   the source directory contains another ‘config.h’, the build directory
   should be searched first.

In INN, we have to use "" for make depend to work, and make depend in turn
exists because we're not using Automake.

That reminds me that I want to take a moment now that everything is in Git
and switch the INN build system over to Automake, since I think it would
get rid of a bunch of maintenance gotchas and chores that currently have
to be done manually.  It will raise a few questions, though, such as
whether the additional human-readable information in MANIFEST is useful
when it's no longer necessary to generate a distribution.

> In HACKING:

>   "clibrary.h" does also include "config.h", but it's somewhat poor form
>   to rely on this; it's better to explicitly list the header
>   dependencies for the benefit of someone else reading the code.

> :)
> We can get rid of that paragraph then.

Yes.

> Also, from TODO:

> * The include files necessary to use libinn, libstorage, and other
>   libraries should be installed in a suitable directory so that other
>   programs can link against them.  All such include files should be under
>   include/inn and included with <inn/header.h>.  All such include files
>   should only depend on other inn/* header files and not on, e.g.,
>   config.h.  All such include files should be careful about namespace to
>   avoid conflicts with other include files used by applications.

> Isn't it already done?
> All headers in include/inn only include headers also in include/inn, which
> never include "config.h".

conffile.h and innperl.h are still at the top level.  (And ppport.h,
although it's a bit of a weird case.)

> I think the namespace is ok (I'm not aware of any complaints about it).
> Unless I am missing something, there isn't any work to do for that task,
> is it?

So, the problem with the namespace is that we create a shared library that
has tons of random functions exported from it that don't share any common
prefix, and hence sprawl all over the C function namespace.  (Stuff like
concat, all the buffer_* stuff, etc.  Often they have their own namespace,
like QIO, but sometimes they don't nad it's not really consistent.

For a well-behaved shared library, ideally all that stuff should have inn_
or INN_ prefixes.  But that's a huge pain in the ass to do because it's
little changes all over the source base, and I'm not sure if it's a good
use of time.  This is part of why, for example, the Debian packages
install the libinn shared library out of the normal library search path,
since it's not entirely a well-behaved shared library.  (Also it doesn't
have symbol versioning.)

Another approach rather than rename everything would be to shrink the
symbols provided by the shared library down considerably to only things
that make sense for external programs seeking to have API access to INN
data files, namespace all of those, and then hide all the other symbols.
This would probably involve creating a separate static library with the
internal utility code like buffer and linking that with all the
executables.  This is what I do for other projects with a shared library
to keep the shared library ABI minimal.  Again, not sure it's worth it,
and it would break any out-of-tree software that really uses libinn for
its random utility functions.

Switching the build system to Automake would make it way easier to do this
sort of thing, so even if we did want to do all of this, we should do that
first.

> Finally, we already have "inn/system.h", with a few configure-time options
> set (like INN_HAVE_INET6 or INN_HAVE__BOOL) needed by INN headers.
> It is notably included by "inn/defines.h" which only include 3 header
> files.  Time has come to get rid of "inn/defines.h" at the same time, 
> and just include the necessary headers instead of it.

Yup, that would be great.

> I also see that the next clang-format release (13) will permit aligning
> array of structs with for instance:
> AlignArrayOfStructures: Right

> struct test demo[] =
> {
>     {56,    23, "hello"},
>     {-1, 93463, "world"},
>     { 7,     5,    "!!"}
> };

> The current formatting breaks them badly...
> I believe this new style option will permit removing a few /* clang-format
> off */ comments in the code.

Oh, excellent, looking forward to that.

Thank you for tackling this reformatting!

-- 
Russ Allbery (eagle at eyrie.org)             <https://www.eyrie.org/~eagle/>

    Please send questions to the list rather than mailing me directly.
     <https://www.eyrie.org/~eagle/faqs/questions.html> explains why.