On Tue, Feb 12, 2019 at 07:21:46PM -0500, Andrew Cagney wrote: > On Tue, 12 Feb 2019 at 11:25, D. Hugh Redelmeier <[email protected]> wrote: > > > > commit 6909918af77cb8cc39bdad12c51543e16f8297a9 > > Author: Paul Wouters <[email protected]> > > 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! _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
