On Monday 24 March 2014 01:09:01 you wrote: > On Monday, 24 March 2014, at 12:06 am, Tito wrote: > > On Sunday 23 March 2014 23:09:59 you wrote: > > > 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. > > > > 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. ;-) > > That would start to get quite messy as the number of options increases: > > enum { > OPT_a = (1 << 0), /* all */ > OPT_d = (1 << 1) * ENABLE_FEATURE_SWAPON_DISCARD, > OPT_f = (1 << (1 + ENABLE_FEATURE_SWAPON_DISCARD)) * > ENABLE_FEATURE_SWAPON_FIXPGSZ, > OPT_p = (1 << (1 + ENABLE_FEATURE_SWAPON_DISCARD + > ENABLE_FEATURE_SWAPON_FIXPGSZ)) * ENABLE_FEATURE_SWAPON_PRI, > OPT_s = (1 << (1 + ENABLE_FEATURE_SWAPON_DISCARD + > ENABLE_FEATURE_SWAPON_FIXPGSZ + ENABLE_FEATURE_SWAPON_PRI)) * > ENABLE_FEATURE_SWAPON_SUMMARY > }; > > Bleh. > > I prefer this approach: > > ret = getopt32(argv, > IF_FEATURE_SWAPON_DISCARD("d::") > IF_FEATURE_SWAPON_FIXPGSZ("f") > IF_FEATURE_SWAPON_PRI("p:") > IF_FEATURE_SWAPON_SUMMARY("s") > "a" > IF_FEATURE_SWAPON_DISCARD(, &discard) > IF_FEATURE_SWAPON_PRI(, &prio) > ); > #if ENABLE_FEATURE_SWAPON_DISCARD > if (ret & 1) { // -d > /* ...handle discard option... */ > } > ret >>= 1; > #endif > #if ENABLE_FEATURE_SWAPON_FIXPGSZ > if (ret & 1) { // -f > /* ...handle fixpgsz option... */ > } > ret >>= 1; > #endif > #if ENABLE_FEATURE_SWAPON_PRI > if (ret & 1) { // -p > /* ...handle priority option... */ > } > ret >>= 1; > #endif > #if ENABLE_FEATURE_SWAPON_SUMMARY > if (ret & 1) { // -s > /* ...handle summary option... */ > } > ret >>= 1; > #endif > if (ret /* & 1: redundant */) { // -a > /* ...handle all option... */ > } > > That's much cleaner and more scalable. >
Hi Matt, if you like it this way it is fine for me, but for example the (ret &1 ) ... ret >>= 1 is harder to read than (ret & OPT_a) as at a first glance it looks always as the same option. For me that way it is easier to read and for some options depending on their needs you could get it done with no ifdefs at all but relaying only on compiler's dead code elimination for size optimization (for example when they have only a flag but no variables). Ciao and have a nice day, Tito int swap_on_off_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE; int swap_on_off_main(int argc UNUSED_PARAM, char **argv) { int ret; const char * option_str = "a"; #if ENABLE_FEATURE_SWAPON_PRI int prio; #endif INIT_G(); if (applet_name[5] == 'n') { option_str = "a" IF_FEATURE_SWAPON_PRI("p:"); if (ENABLE_FEATURE_SWAPON_PRI) opt_complementary = "p+"; } ret = getopt32(argv, option_str IF_FEATURE_SWAPON_PRI(, &prio)); #if ENABLE_FEATURE_SWAPON_PRI if (ret & OPT_p) { /* priority is a value between 0 and 32767 */ g_flags = SWAP_FLAG_PREFER | MIN(prio, SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT) << SWAP_FLAG_PRIO_SHIFT; } #endif if (ret & OPT_a) return do_em_all(); argv += optind; if (!*argv) bb_show_usage(); /* ret = 0; redundant */ do { ret += swap_enable_disable(*argv); } while (*++argv); return ret; } _______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox