[Swan-dev] making struct finite_state part of struct state

[Andrew Cagney]

Heads up.

I'm about to push a change renaming .st_finite_state to .st_state;
inline all the wrapper macros such as .st_state_name; and drop the
.fs_ prefixes.  While a lot of code gets cleaned up vis:

-                                           enum_name(&state_names,
-                                                     st->st_state
-                                                     )));
+                                       st->st_state->name));

and:

-                               lswlogf(buf, "%s: %s",
st->st_finite_state->fs_name,
-                                       st->st_finite_state->fs_story);
+                               lswlogf(buf, "%s: %s", st->st_state->name,
+                                       st->st_state->story);


it doesn't try to fix code like:

-       bool responder = (st->st_state != STATE_PARENT_I2);
+       bool responder = (st->st_state->kind != STATE_PARENT_I2);

where the "correct fix" is to instead use attributes such as
st->st_sa_role or md->message_role.  Later for that.

On Thu, 21 Feb 2019 at 10:40, Antony Antony <antony at phenome.org> wrote:
>
> Hi Andrew,
>
> On Wed, Feb 20, 2019 at 09:22:52AM -0500, Andrew Cagney wrote:
> > This continues a face-to-face discussion from last year.
>
> I re-collect such a discussion from last fall.
> If you are thinking of fixing only because of our discussion, then please do
> not change.
>
> I tried to convey my annoyance of unexpected change of a
> well known variable to a #define! The new one was hard to use in gdb.
> It is long, "st->st_state" vs "st->st_finite_state->fs_kind".
> Also code used short version at many places, mixed usage was annoying.
> Now mostly the long version. If we are not mixing them or rename every 6
> months I am ok:)
>
> I was suggesting to replace all instances of short form at once. However
> when I talked to you I got the impression that your preference was to change
> inclemently. And re-write often! The use st->st_state is disappearing now?
>
> f937038e9d
> +#define st_state st_finite_state->fs_state
> The commit was over a year ago:)
>
> Over time the use of st->st_state shrunk. However, more variants appeared:)
> see 179bf3901. I prefer one version for a well know variable name. It is
> probably a matter of taste!
>
> > It was pointed out that one downside of replacing 'enum state_kind'
> > with 'struct finite_state' is that when a 'struct state' is printed
> > using a debugger it no longer shows the 'state' as an enum.
>
> And now this proposal sounds like just when I am getting used to the long
> form there may be another change. Thanks for the heads up!
>
> > Off hand I can think of two solutions:
> >
> > - redundantly store both a 'struct finite_state' pointer, and an 'enum
> > state_kind' in 'struct state'
> >
> > - store a copy of 'struct finite_state' in 'struct state'
> >
> > My preference is for the second
>
> my preference is fewer "defines" for well known variables. such as st_state
> or say st_serial_no.
>
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev