On Sunday 23 March 2014 23:09:59 you wrote:
> On Sunday, 23 March 2014, at 8:29 pm, Tito wrote:
> > On Sunday 23 March 2014 00:33:04 Matt Whitlock wrote:
> > > Attached are a series of three patches affecting swaponoff.
> >
> > Hi,
> > attached you will find a alternative patch to your patches #1 and #2 that
> > includes your fixes and
> > improvements and reworks swap_on_off_main to allow you to implement patch
> > #3 without
> > creating a #ifdef hell.
> > I hope that the code is also easier readable than the original
> > unpatched version.
>
> So are the main improvements here the use of the IF_FEATURE* macros and
> testing the ENABLE_FEATURE* macros at the C level rather than at the
> preprocessor level? This is still going to create a "hell" when adding the
> discard flag. Consider what will happen in the enum: if DISCARD is enabled
> but PRI is disabled, then OPT_d will be (1 << 1), but if both are enabled,
> then OPT_p will be (1 << 1) and OPT_d will be (1 << 2) (or vice versa).
> Imagine adding five more optional features. It leads to a combinatorial
> explosion. It's cleaner to downshift the bitmap after checking each bit
> within its appropriate #if section.
>
> I do agree that I could clean up the construction of the options string using
> the IF_FEATURE* macros. See attached patch.
Hi,
looks good to me.
The problem with flags could be solved easily, take a look at how it is done in
df.c:
enum {
OPT_KILO = (1 << 0),
OPT_POSIX = (1 << 1),
OPT_ALL = (1 << 2) * ENABLE_FEATURE_DF_FANCY,
OPT_INODE = (1 << 3) * ENABLE_FEATURE_DF_FANCY,
OPT_BSIZE = (1 << 4) * ENABLE_FEATURE_DF_FANCY,
OPT_HUMAN = (1 << (2 + 3*ENABLE_FEATURE_DF_FANCY)) *
ENABLE_FEATURE_HUMAN_READABLE,
OPT_MEGA = (1 << (3 + 3*ENABLE_FEATURE_DF_FANCY)) *
ENABLE_FEATURE_HUMAN_READABLE,
};
that way the enum is independent from the combination of options enabled and
most #ifdefs could be removed
and we are lucky becuase we have only 3 flags. ;-)
Ciao,
Tito
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
