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

[Andrew Cagney]

On Thu, 23 May 2019 at 07:20, Antony Antony <antony at phenome.org> wrote:
>
> Hi Andrew,
>
> thanks for the heads up.
>
> wow, that is an interesting change! At first glance it appears quite
> different from the Feb 22 proposal. I guess this is better!
> I say push it now, sooner than later.

There was general push back on my suggestion to copy FS into state,
and a preference for flushing the compat macros as they just made code
confusing (and debugger support still isn't really there).

> I am wondering "st_fs" instead "st_state" would be better? your call! You
> probably evaluated shorter names and discarded them:)

I wondered about that; along with .st_fs->fs_...  Main reason for
st_state is to preserve a name we know (annoyingly the compiler
doesn't complain about st->st_state == STATE_UNKNOWN so I had to find
them manually).

> Would you be able to do it in one commit?
> I think this change will not to affect any output, or test case reference
> outputs. However, IMHO the code change is pretty big.
> This is why I am in favor to push it now than waiting. Otherwise it would be
> painful for you and others.

It's one commit.

> Lets start a new thread to serialize changes the usual suspects want to push
> post 3.28, in the next couple of weeks. Some of us have 6 months of
> finished, or almost finished branches to push. Such as xfrmi, Andrew O(1)
> patches, this one..
>
> -antony
>
>
> On Wed, May 22, 2019 at 09:09:39PM -0400, Andrew Cagney wrote:
> > 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