This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: cpplib: command line argument parsing
- To: Neil Booth <NeilB at earthling dot net>
- Subject: Re: cpplib: command line argument parsing
- From: Zack Weinberg <zack at wolery dot cumb dot org>
- Date: Sun, 20 Feb 2000 22:48:02 -0800
- Cc: gcc-patches at gcc dot gnu dot org
- References: <E12LR6w-0000ta-00@monkey.rosenet.ne.jp>
On Thu, Feb 17, 2000 at 10:36:50PM +0900, Neil Booth wrote:
> This is the current state of my patch to tighten up cpplib's command
> line handling.
It's really getting there! I do keep nitpicking, but the basic idea
is right on the money.
> o I'm not sure if the internationalisation stuff is quite right -
> could you please confirm if it's what you had in mind, Zack?
More or less. I don't like the magic numbers.
> o The -W options are strcmp-ed one by one; this may be worth improving
> with a binary search. Please advise.
It is acceptable for the moment, as it's no worse than we have now.
But please move -Wall to the top and -Wtraditional to second place;
these are the most commonly used.
We can revisit this once the basic patch is in.
> o -ifoutput is still accepted
Since it's still accepted in the main tree, this is fine.
I have a couple more kibitzes:
> * cppinit.c (opt_comp) New function. Called by sort_options
> as the comparison routine.
> (sort_options) New function. Sorts the array of command line
> options at run time for non-ASCII host environments.
> (parse_option) New function. Parses a single command line
> option, possibly taking an argument.
You don't need to say what new functions do in the changelog. That
goes in the comment above the function, in the code.
> (cpp_handle_option) Rewritten to use parse_option for binary
> searching of options.
> (cpp_handle_options) Sorts the array of command line options
> during its first invocation.
Imperative voice:
(cpp_handle_option): On first invocation, sort the array of
command line options.
> (print_help) Updated run-time help.
This suffices:
(print_help) Update.
>
> Index: cppinit.c
> ===================================================================
> RCS file: /cvs/gcc/egcs/gcc/cppinit.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 cppinit.c
> --- cppinit.c 2000/02/15 16:36:32 1.41
> +++ cppinit.c 2000/02/17 13:18:26
> @@ -201,6 +201,13 @@ static void initialize_dependency_output
> static void new_pending_define PARAMS ((struct cpp_options *,
> const char *));
>
> +#ifdef HOST_EBCDIC
> +static int opt_comp PARAMS ((const void*, const void*));
> +#endif
> +static void sort_options PARAMS ((void));
Just call qsort directly, and see below.
...
> +struct cl_option
> +{
> + const char* opt_text;
> + int arg_index; /* -1 = takes no argument */
> + size_t opt_len;
> + enum opt_code opt_code;
> +};
On 64-bit machines, this structure will have unnecessary padding. Put
opt_len before arg_index.
> +
> +/* Run-time sorted array of pointers to command-line options below */
> +static const struct cl_option* cl_options[N_OPTS];
Don't introduce this extra layer of indirection. qsort is quite
capable of sorting the cl_opt_array.
...
> +/* These lists must be ASCII sorted. Make enum order above match this. */
> +#define DEF_OPT(text, msg_idx, code) {text, msg_idx, sizeof(text) - 1, code}
/* On EBCDIC platforms, this list must be sorted at runtime. */
#ifdef HOST_EBCDIC
static struct cl_option cl_opt_array =
#else
> +static const struct cl_option cl_opt_array[] =
#endif
> +{
> + DEF_OPT("", -1, OPT_stdin_stdout),
...
> + DEF_OPT("W", 0, OPT_W), /* NB: Immed arg only, and not reqd */
Word wrap to 78 columns, or shorten the comment.
...
> int i;
> int strings_processed;
#ifdef HOST_EBCDIC
> + static int opts_sorted = 0;
> +
> + if (!opts_sorted)
> + {
> + opts_sorted = 1;
> + sort_options();
Even if the function has no arguments, you must put a space between
the name and the open paren. (No, I don't like it either.)
> + }
#endif