[Swan-dev] modularity erosion

[D. Hugh Redelmeier]

(Sorry about putting [swan-commit] in the original Subject: line.)

| From: Antony Antony <antony at phenome.org>

| 1st approach, I think what Hugh is proposing, is sprinkle a bunch
| declarations in .c or .h. In our code sometimes they are clearly
| commented with /* forward declaration */

That's not what I'm proposing.

If you define a variable or a struct field with a pointer-to-struct
type, the struct type's definition need not be in the scope of the
definition of the variable or field.  Like this:

struct fred {
	struct joe *p;	/* struct joe need not be defined */
};

If you define a variable or a struct field with a struct type, the 
the definition needs to be within the scope of the definition of the
struct type.  You can think of this as the compiler needing to know
the size of the struct type.

struct fred {
	struct joe p;	/* struct joe needs to have been defined */
};

Any code that accesses struct fields must be within the scope of a 
definition of the struct.

But most of pluto doesn't need to access (directly) any substructure
of ike_proposals.  It would be best if any such (ill-disciplined or
buggy) reference were flagged by the compiler.

If code needs to reference the substructure of ike_proposals, it would 
signify this by the positive action of #include "proposal.h"

This idea of opaque types is fundamental to modern programming but only 
modestly supported by C.  We should aim at writing good code, in spite of 
C, if that is necessary.

| 2nd approach is move around definitions of struct. I imagine this is 
| Andrew's choice. Atleast in this case?

I'm not sure what you mean here.

There is a possible example if struct {ike,child}_proposals were
wrappers for a pointers to struct proposal, but that's pretty jarring.

| 3rd Me sometimes and Paul, guess often, use #include foo.h
| includes every where:) No declarations for struct.
| That means to avoid "error: redefinition" add
|  #ifndef _FOO_, #define _FOO_ at the top of include file.
| I have also used forward declarations, however after long time I am not so 
| sure if that is best for Libreswan.

A struct only needs a Forward Declarations if the first use is in a
parameter of a function prototype.  Without the forward declaration,
the the scope is limited to the function prototype, and that's just not
useful (smart compilers warn you).

So:
struct thing;	/* forward *

extern void box(struct thing *p);

It just doesn't come up elsewhere (I could contrive an example).

So it is kind of easy.  And if you get it wrong, the compiler will
probably tell you.  Here's an example from GCC.

extern j(struct xxx *d);

t.c:1:17: warning: 'struct xxx' declared inside parameter list will not be visible outside of this definition or declaration


| I am wondering what are the pros and cons of the three approaches
| now a days form Libreswan perspective. Lets focus on Libreswan perspective,  
| something of this size complexity and not kernel... or apache.. or perl...

Summary: don't let code be in the scope of declarations it ought not
to use.  It only invites accidents and bad modularity.

| What I gathered so far:
| I often wonder why 1st approach is better.

Reduce the soup of visible declarations.

| I notice side a few effects of the first method.
| We don't clean up unused declarations. They seems to linger around when code 
| move around or that specific declaration is not necessary anymore in that 
| particular location. Sometimes we include ".h" anyway to get definition and 
| the declaration lingers.

It takes work to clean up.  Cleaning up is most of what I do these
days.

| The compiler do not seems to complain about duplicate declarations.

That's a Good Thing.

It does complain about duplicate definitions.

The distinction between declarations and definitions is sometimes
subtle.  Sometimes retroactive!

| Or linker do not complain about declarations without matching definitions 
| else where after linking.

If the declaration isn't actually used then nobody complains if there
is no definition.  Too bad.  But if there were diagnostics, we'd
probably be complaining about them too.  (Andrew complained on IRC
yesterday that -Wunused is too noisy.)

| Also when things get renamed, declarations would linger around.
| Is there an easy way to find unused local declarations? more tools?

It is a bit tedious.  Hints:

- "git grep" will search our whole tree.

- \< and \> extended regular expression operators are helpful forcing
  exact matches of identifiers

- grep's option -n will list line numbers with the matches.

- EMACS family editors have commands like "parse-errors" that let you
  jump easily between matches listed by grep -n.  Surely VIM has
  something like this.

- I use a very simple JOVE editor macro to combine all this in a
  convenient package.  EMACS on VIM ought to let you do this too.

I also have spent some time eliminating useless #includes, but it
takes too much effort.  Delete a #include; compile; take appropriate
action if it fails (either delete even more or restore the #include).

| Googling suggest using forward declarations compared to include file would 
| reduce compile time. Then I am curious to see numbers for a project like 
| Libreswan. I am wondering is it a big issue in Libreswan in 2019?

No.  But faster is better.

| Another argument in favor forward declaration is name space collision.  
| Again wondering is that a real issue for Libreswan. May be in some cases.

I sure hope not.

| Second option of moving around definitions is not so great in my opinion:) 
| from what I notice there is a lot of code churn and definitions. Soon that 
| tend to appear like magic coming from under the snow:) You add one line of 
| code then you have more around definitions instead possibly #include new file 
| or two.

Modularity is an art.  Some things belong together logically; some
don't.  We should all have enough experience to do a good job.
Sometimes someone notices how to make improvements.

| PS: a side rant name space collisions. I can understand name space 
| separation between lib and pluto. However, if we start using function names 
| or sturct names several times in pluto and isolate the name space across 
| files it become hard to read code and debug using gdb, remember how we used 
| get confused with log functions! or there would be 5 libreswanlog() 
| functions, one would wonder when to use which, aslo debugging where to put 
| break points. It is good to see log functions getting better!

I would like our names to not overlap.

But sometimes it is intentional: the same code can work in two
different environments because each has a (distinct) implementation of
the same abstraction.