Re: [PATCH] parseopt: introduce OPT_RECURSE to specify shared options

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]

On Mon, Nov 05, 2007 at 12:09:41PM +0000, Johannes Schindelin wrote:
> 
> The diff options should not need to be defined in every user of the
> diffcore.  This provides the framework:
> 
> 	extern struct option *diff_options;
> 
> 	struct option options[] = {
> 		OPT_RECURSE(diff_options),
> 		...
> 		OPT_END(),
> 	};
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	This is not yet clever enough to show the correct usage when
> 	some sub option uses the same short or long name as one that
> 	has already been printed.
> 
> 	Add the superlevel options struct as another parameter to
> 	usage_show_options(), and write an unset_option() function
> 	which takes a short name, a long name, and that superlevel
> 	options struct, and unsets all options which match.

  Alternatively you can have a 256-bit bitfield to mark short options
that have already been seen and a hashtable of long options already
printed out, and while outputting an option, one should check that
either the short option or the long option is new.

  This is nicer as some of the struct option may live in .rodata

>  parse-options.c          |   68 +++++++++++++++++++++++++++++++++++-----------
>  parse-options.h          |    2 +
>  t/t0040-parse-options.sh |   24 ++++++++++++++++
>  test-parse-options.c     |   10 +++++++
>  4 files changed, 88 insertions(+), 16 deletions(-)

  Okay, we discussed this with Johannes on IRC. I came up with the
relocation thing because I feared that the msys port (and maybe other ?)
that are about to use (or already do) threads would step on each other
toes while recursing into a sub-array of options.

  Johannes thinks that this never happens in our codebase, hence that my
patches are an overkill.

  The likely users of this feature are currently diff options (diff.c
diff_opt_parse) and revisions (builtin-log.c setup_revisions).

  Using Johannes patch, we will have to export a global struct
diff_option (resp. struct rev_info) from diff.c (resp. revisions.c) and
no function (or almost) would take struct diff_option (resp struct
rev_info) as an argument because everyone would work on the global
variable[0].

  With my patches, we can work like we do now, with a more functional
approach.

  I like the kind of code that I allow to write better (I tend to
dislike big fat global variables), though it's obvious that Johannes
patch is a lot simpler and I like that.


  [0] actually we don't *need* to remove the struct diff_options argument
      from many functions except from diff_opt_parse, it's just that for
      real, everybody will work on the same global structure anyway.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]