[Swan-dev] [Swan-commit] modularity erosion

[Antony Antony]

On Tue, Feb 12, 2019 at 07:21:46PM -0500, Andrew Cagney wrote:
> On Tue, 12 Feb 2019 at 11:25, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
> >
> > commit 6909918af77cb8cc39bdad12c51543e16f8297a9
> > Author: Paul Wouters <pwouters at redhat.com>
> > Date:   Mon Feb 11 19:26:40 2019 -0500
> >
> >     pluto: removal all but one include of proposals.h
> >
> >     Since connections.h needs it, and that is included everywhere else,
> >     there is no need for separate includes.
> >
> > If this becomes a trend, this is terrible.
> 
> I suspect a lot weren't needed before this change - #includes seem to breed,
> 
> > In this particular case, it seems forced:
> >
> > struct connection has:
> >
> >         struct ike_proposals ike_proposals;
> >         struct child_proposals child_proposals;
> 
> The fields are shared between IKEv1 and IKEv2.
> 
> The old code had something like:
>     struct {ike,esp}_alg_info {
>       struct alg_info ai;
>     }
> (yes esp, was used by ah)
> 
> which pretty much forced everything to include the header anyway so
> that they could pass &ai.
> 
> Anyway, a cleanup I've somewhere in my queue is to move the definition
> of struct {ike,child}_proposal to connection.h, they're not used by
> the proposal code proper. 

I am curious about what Hugh brought up here. I would love to learn more the 
issue and solutions. I think there are three methods of fixing such issues 
in our code.

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 */

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

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.

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...

Any pointers to read up would be great.

What I gathered so far:
I often wonder why 1st approach is better.
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.
The compiler do not seems to complain about duplicate declarations.
Or linker do not complain about declarations without matching definitions 
else where after linking.
Also when things get renamed, declarations would linger around.
Is there an easy way to find unused local declarations? more tools?

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?
Another argument in favor forward declaration is name space collision.  
Again wondering is that a real issue for Libreswan. May be in some cases.

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.

If it is all matter of taste, setting a best practice would help. Hopeful 
not second method:) 

-antony

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!