simple command line parser

[Greg B]

Well since getopt() doesn't seem to be compatible with Windows, and
the free implementation of it for Windows that I found still had some
annoying restrictions, I thought I'd whip up a simple parser myself.
Just wanted to see if anyone could provide me with some constructive
criticism :) any feedback would be greatly appreciated
-----------------------------------------------------------------------------

#include "stdio.h"
#include "string.h"
#include "stdlib.h"

#define MAX_OPTION_ARGS 10

enum optionName {
Start,
Stop,
Remove,
Install,
Debug,
Num_Opts
};

typedef struct optionStruct {
char *name;
int expNumVal;
char *val[MAX_OPTION_ARGS];
int valid;
} optStruct;

optStruct optData[Num_Opts] = {
{ "-start", 1 },
{ "-stop", 2 },
{ "-remove", 2 },
{ "-install", 1 },
{ "-debug", 3 }
};

int usage(void)
{
printf("Usage is as follows...");
return 0;
}

int StartService(ch ar * service_name)
{
return 0;
}

int main (int argc, char * argv[])
{
int a, b, c;
const char *optstring;
const struct option *longopts;
int *longindex;

for (a = 1; a < argc; a++)
{
for (b = 0; b < Num_Opts; b++)
{
if (strcmp(argv[a], optData[b].name) == 0)
{
if (a + optData[b].expNumVal< argc)
{
for (c = 0; c < optData[b].expNumVal; c++)
{
optData[b].val[c] = argv[a + c + 1];
}
optData[b].valid = 1;
a += optData[b].expNumVal;
}
else
{
fprintf(stderr, "ERROR: insufficient number of arguments for
option %s\n", optData[b].name);
usage();
return 1;
}

break;
}
}

if (b == Num_Opts)
fprintf(stderr, "Warning: unknown option %s (ignored)\n", argv[a]);
}
/*
Sample usage...

if (optData[Start].valid == 1)
{
StartService(op tData[Start].val[0]);

}
*/
return 0;
}

[Irrwahn Grausewitz]

ru******@hotmai l.com (Greg B) wrote in
<f5************ **************@ posting.google. com>:

Well since getopt() doesn't seem to be compatible with Windows, and
the free implementation of it for Windows that I found still had some
annoying restrictions, I thought I'd whip up a simple parser myself.
Just wanted to see if anyone could provide me with some constructive
criticism :) any feedback would be greatly appreciated
-----------------------------------------------------------------------------

#include "stdio.h"
#include "string.h"
#include "stdlib.h"

#define MAX_OPTION_ARGS 10

enum optionName {
Start,
Stop,
Remove,
Install,
Debug,
Num_Opts
};

typedef struct optionStruct {
char *name;
int expNumVal;
char *val[MAX_OPTION_ARGS];
That's what I'd call an annoying restriction. :)
Why not char *val and dynamic memory allocation?
int valid;
} optStruct;

optStruct optData[Num_Opts] = {
{ "-start", 1 },
{ "-stop", 2 },
{ "-remove", 2 },
{ "-install", 1 },
{ "-debug", 3 }
}; Nice, you include the dash with every option name. :)

int usage(void)
{
printf("Usage is as follows...");
return 0;
}

int StartService(ch ar * service_name)
{
return 0;
}
Please, don't use TABs for formatting code you post in usenet -
this wastes space and garbles the lines.
<code gently reformatted>int main (int argc, char * argv[])
{
int a, b, c;
const char *optstring;
const struct option *longopts;
int *longindex;

for (a = 1; a < argc; a++)
{
for (b = 0; b < Num_Opts; b++)
{
if (strcmp(argv[a], optData[b].name) == 0)
{
if (a + optData[b].expNumVal< argc)
{
for (c = 0; c < optData[b].expNumVal; c++)
{
optData[b].val[c] = argv[a + c + 1];
}
optData[b].valid = 1;
a += optData[b].expNumVal;
}
else
{
fprintf(stderr, "ERROR: insufficient number of arguments "
"for option %s\n", optData[b].name);
usage();
return 1;
}
break; This break has absolutely no chance of beeing executed!
}
}
if (b == Num_Opts)
fprintf(stderr, "Warning: unknown option %s (ignored)\n", argv[a]);
}
/*
Sample usage...

if (optData[Start].valid == 1)
{
StartService(op tData[Start].val[0]);
}
*/
return 0;
}

Sorry, if my critique is somewhat destructive, but this is one of
the most complex option parsers I've been looking at. Anyway, if it
meets your needs, use it, but it might be a good idea to capsule it
in a separate module in order to keep your main function tidy and
clean. And you should consider not to make excessive use of global
variables, as this may lead to unwanted interferences eventually.

Btw: did you compile it? This is what I got when I did:

gcc.exe "D:\Temp\ngetop t.c" -o "D:\Temp\ngetop t.exe" -W -Wall
-Wunreachable-code -ansi -O3 -g3 -I"C:\Programme\ DevCpp5b8\inclu de"
-L"C:\Programme\ DevCpp5b8\lib"

ngetopt.c:25: warning: missing initializer
ngetopt.c:25: warning: (near initialization for `optData[0].val')
ngetopt.c:26: warning: missing initializer
ngetopt.c:26: warning: (near initialization for `optData[1].val')
ngetopt.c:27: warning: missing initializer
ngetopt.c:27: warning: (near initialization for `optData[2].val')
ngetopt.c:28: warning: missing initializer
ngetopt.c:28: warning: (near initialization for `optData[3].val')
ngetopt.c:29: warning: missing initializer
ngetopt.c:29: warning: (near initialization for `optData[4].val')
ngetopt.c:46: warning: unused variable `optstring'
ngetopt.c:47: warning: unused variable `longopts'
ngetopt.c:48: warning: unused variable `longindex'
ngetopt.c:72: warning: will never be executed
ngetopt.c:38: warning: unused parameter `service_name'

If one takes warnings as errors (at least, I do), this is a /bit/
more than I'd expect from 86 lines of my code (though there are
no severe warnings, and you can get rid of them very easily).

I hope my reply wasn't to harsh... :)

Regards,
Irrwahn

--
When things look dark,
hold your head high so it can rain up your nose.

[Eric Sosman]

Irrwahn Grausewitz wrote:

ru******@hotmai l.com (Greg B) wrote:

for (b = 0; b < Num_Opts; b++)
{
if (strcmp(argv[a], optData[b].name) == 0)
{
if (a + optData[b].expNumVal< argc)
{
for (c = 0; c < optData[b].expNumVal; c++)
{
optData[b].val[c] = argv[a + c + 1];
}
optData[b].valid = 1;
a += optData[b].expNumVal;
}
else
{
fprintf(stderr, "ERROR: insufficient number of arguments "
"for option %s\n", optData[b].name);
usage();
return 1;
}
break;

This break has absolutely no chance of beeing executed!

Why not?

--
Er*********@sun .com

[Irrwahn Grausewitz]

Eric Sosman <Er*********@su n.com> wrote in <3F************ ***@sun.com>:

Irrwahn Grausewitz wrote:

ru******@hotmai l.com (Greg B) wrote:

> break;

This break has absolutely no chance of beeing executed!

Why not?

Urgh, sorry, my fault. I'll go looking for my glasses... :)

--
Air is water with holes in it.

[Alex]

Greg B <ru******@hotma il.com> wrote:

Well since getopt() doesn't seem to be compatible with Windows, and
the free implementation of it for Windows that I found still had some
annoying restrictions, I thought I'd whip up a simple parser myself.
Just wanted to see if anyone could provide me with some constructive
criticism :) any feedback would be greatly appreciated
----------------------------------------------------------------------------- #include "stdio.h"
#include "string.h"
#include "stdlib.h"
Why are these in double-quotes? It would be more reasonable to
expect the compiler to look for these headers in their proper
locations to start with, not in the current directory.
#define MAX_OPTION_ARGS 10 enum optionName {
Start,
Stop,
Remove,
Install,
Debug,
Num_Opts
};
This has nothing to do with the rest of your code.
typedef struct optionStruct {
char *name;
int expNumVal;
char *val[MAX_OPTION_ARGS];
int valid;
} optStruct;
If you are going to be spelling out 'Struct' anyway, then why not
just lose the typedef? You will be saving yourself a shift-key hit.
If you insist on using the typedef, you do not need the structure
identifier, since you are not using it.
optStruct optData[Num_Opts] = {
{ "-start", 1 },
{ "-stop", 2 },
{ "-remove", 2 },
{ "-install", 1 },
{ "-debug", 3 }
};
Why not make the option prefix configurable via a define or somesuch?
Additionally, you should be initializing 'valid'.
int usage(void)
{
printf("Usage is as follows...");
return 0;
}
This is not terribly useful.
int StartService(ch ar * service_name)
{
return 0;
}
Neither is this.

Here is your reindented code. Still unfitting line length for Usenet,
I'm afraid.
int
main (int argc, char *argv[])
{
int a, b, c;
const char *optstring;
You don't use this.
const struct option *longopts;
What is 'struct option'? You haven't defined it.
int *longindex;
You don't use this.
for (a = 1; a < argc; a++)
{
for (b = 0; b < Num_Opts; b++)
{
if (strcmp (argv[a], optData[b].name) == 0)
{
if (a + optData[b].expNumVal < argc)
{
This is where it gets really silly. You take the index of the
argv element you are parsing and add an offset to produce the
index of an expected supplementary value?
for (c = 0; c < optData[b].expNumVal; c++)
{
optData[b].val[c] = argv[a + c + 1];
}
optData[b].valid = 1;
a += optData[b].expNumVal;

Hmmm...

< snip the rest >

This is the most complicated and constrained way of getting
command-line arguments that I have ever seen.

You need to rethink your design by properly defining the problem
domain.

- What kind of options are you going to be parsing?

- Will there be only boolean options or will you have to handle
various supplementary types such as strings, integers, floats
and so on?

- What will happen if an option has incorrect syntax or a
a mandatory option is missing?

- Do you want to copy and paste code for each new program you
write or do you want to make a universally usable library
function, which will be configurable by the user?

To get you started, here is what I think your options structure
should look like.

enum opt_t { BOOL, INT, STR /*, etc */ }

struct option
{
char *opt; /* option name */
opt_t type; /* option type */
char mandatory; /* is option mandatory ? */
char *usage; /* usage for this specific option */
void *value; /* parsed value or null */
}

....only the last element is an output of the parser.

In your client code, you will populate an array of 'struct
option' and pass it to your parser along with argc and argv.

At any time if the parser fails, it will print out a program
usage based on the 'usage' fields in the array.

You should also leave it up the client code to define the prefix

Hope this helps.

Alex