Closed Bug 346354 Opened 13 years ago Closed 12 years ago

add capability to parse long command line option names

Categories

(NSPR :: NSPR, enhancement, P2)

4.6.1
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alvolkov.bgs, Assigned: nelson)

References

Details

(Whiteboard: PKIX)

Attachments

(4 files, 9 obsolete files)

2.92 KB, text/plain
Details
1.98 KB, patch
wtc
: review+
Details | Diff | Splinter Review
810 bytes, text/plain
Details
13.39 KB, patch
wtc
: review+
Details | Diff | Splinter Review
certutil used out all possible letter/number options. Need to extend
SECU parser to understand string command like options. Since SECU_ParseCommandLine uses functions from nspr(plgetopt.h), I think nspr parser would be the best place to do modification while maintaining backward compatibility.

Currently, PL_GetNextOpt can only parse command line options defined by chars.
Attached patch initial patch (obsolete) — Splinter Review
working patch.
Attachment #231155 - Flags: review?(wtchang)
Comment on attachment 231155 [details] [diff] [review]
initial patch

Alexei,

I only reviewed the patch for style and not for correctness.
Please ask Nelson to review the patch, too.

I suggest some name changes.

"PLOptStr"  ==>  "PLStrOpt" or "PLStringOpt"

"PL_CreateOptState_StrOpt"  ==>  "PL_CreateStrOptState" or
                                 "PL_CreateStringOptState"

(NSPR seldom uses two underscores in names.)

NSPR's indentation level is four spaces, not two spaces.


>-    if (NULL == options)
>-        PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
>-    else
>     {
>         opt = PR_NEWZAP(PLOptState);

Please remove the remaining outer braces and fix the
indentation.  I know this will create more diffs, but
it makes the new code look better.

What does "string option" mean?
Wan-teh,

Thanks for name suggestions.

"String option" should be rather "String Option State". It is a state that is created to control parsing process of string like command line options( "-test -dir a" for example).

> NSPR's indentation level is four spaces, not two spaces.

Most NSPR files have the following editor instruction line as a first line:

/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

which means that a file has indent of 2 spaces, not 4. It is most probably need to be changed to 4.

 
(In reply to comment #3)
> "String option" should be rather "String Option State". It is a state that is
> created to control parsing process of string like command line options( "-test
> -dir a" for example).
> 
I should take it back. "String option" (PLStrOpt) is the structure to define
a possible set of options and for parser, similar to what line "X:" whould do
for command option defined by the char "X". 
Attached patch corected patch (obsolete) — Splinter Review
Assignee: wtchang → alexei.volkov.bugs
Attachment #231155 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #231790 - Flags: review?(nelson)
Attachment #231155 - Flags: review?(wtchang)
Alexei, I don't use emacs, so the incorrect editor instruction
didn't bother me.  However, I found that LXR also uses the emacs
editor instruction (in particular tab-width), so if you see any
incorrect editor instruction, please fix it.

Note that most NSPR source files use a tab-width of 8, like NSS.
But a few NSPR source files use a tab-width of 4.
Comment on attachment 231790 [details] [diff] [review]
corected patch

I emailed my review comments to Alexei.
Attachment #231790 - Flags: review?(nelson) → review-
Priority: -- → P2
Target Milestone: --- → 4.6.3
QA Contact: wtchang → nspr
Attached patch working patch (obsolete) — Splinter Review
Two function have been added
PL_CreateStrOptState - creates PLOptState and sets strOptions(member of PLOptionInternal structure) to a pointer to a null-terminated block of memory allocated for sequence of PLStrOpt structure.
PL_GetNextStrOpt - finds next option and sets PLOptState::option to a sequential number of the option that was found and PLOptState::value to the option's argument.
Attachment #231790 - Attachment is obsolete: true
Attachment #253216 - Flags: review?(nelson)
Attached file possible usage (obsolete) —
Comment on attachment 253218 [details]
possible usage

clearing patch flag. this is not a patch.
Not a problem, except it confuses bugzilla.
Attachment #253218 - Attachment is patch: false
Comment on attachment 253216 [details] [diff] [review]
working patch

I wrote up a long review about this patch, and my browser died before I submitted it, so I lost all that work.  Rather than type it all again, 
I will go over this patch with Alexei by phone.  

Here's one quick observation.

>+PR_IMPLEMENT(PLOptState*) PL_CreateStrOptState(
>+    PRIntn argc, char **argv, const PLStrOpt *options)
>+{
>+    PLOptState *opt = NULL;
>+    
>+    if (NULL != optionsPtr) {
>+        opt = PL_CreateOptState(argc, argv, NULL);

The above call will always return NULL, so this can't be a working patch.
Attachment #253216 - Flags: review?(nelson) → review-
Whiteboard: PKIX
Attached patch third try (obsolete) — Splinter Review
sorry, wrong patch was attached previously.
Attachment #253216 - Attachment is obsolete: true
Attachment #265024 - Flags: review?(nelson)
Attachment #253218 - Attachment is obsolete: true
Attachment #265025 - Attachment mime type: text/x-csrc → text/plain
Blocks: 324740
Comment on attachment 265024 [details] [diff] [review]
third try

the code is not backward compatible as does not allow to parse option string that is a set of single char options concatenated together("-a -c -d" vc "-abc").
Attachment #265024 - Attachment is obsolete: true
Attachment #265024 - Flags: review?(nelson)
Assignee: alexei.volkov.bugs → nelson
I attached the preceeding patch to generate some discussion about how a 
new enhanced version of the PL_GetNextOpt functions should work.  

Here is an English description of the patch.

The patch adds one new function, and one new struct type. It also adds some
members to two existing struct types.  The PLOptState struct is extended
in a way that remains binary compatible.  More on this below.

The function PL_CreateOptState is unchanged, except for the newly added
members of the structure that it returns.  It returns struct PLOptState,
as before.  

The functions PL_GetNextOpt and PL_DestroyOptState are unchanged, in 
that they take the same arguments as before and return compatible results.

The new function, PL_CreateLongOptState, takes the same arguments as 
PL_CreateOptState, plus one additional argument, a pointer to an array of 
PLLongOpt structures.  When that pointer is null, PL_CreateLongOptState 
behaves exactly like PL_CreateOptState.  When it is non-NULL, it will 
cause PL_GetNextOpt to understand options that begin with "--" in a new way.

When the OptState has been created with a non-NULL PLLongOpt pointer, the 
parse will recognize the option consisting of only "--" as the end of the 
input options.  All subsequent members of argv are taken to be positional
parameters, and any leading - characters are ignored thereafter.

Each PLLongOpt struct in the array contains a pointer to a keyword string.
A PLLongOpt struct with a NULL keyword pointer marks the end of the array.
The array may have any non-negative number of valid PLLongOpt structures 
(including zero) before the special null entry that marks the end. 

Each PLLongOpt structure tells the parser if an argument is expected/required
for that option, and also specifies a numeric value to be returned by 
PL_GetNextOpt in the PLOptState.  

The numeric value is an int.  I wanted to return it in the same place in 
the struct where PL_GetNextOpt returns the value for single character 
options, but that value was defined as a "char", so I could not change
it without breaking binary compatibility. So I had to create a new place
in PLOptState to return the value for "long" options.  

When a long-style option is encountered, the "char" return value is zero,
as it also is for positional parameters.  In this case, the new struct
members must be consulted to see if the value is actually a long option.

Note that long options CANNOT occur unless the OptState was created with 
PL_CreateLongOptState and  a non-NULL array of long opts.

Given a new long option with the keyword "opt", the following syntaxes 
are honored:

--opt           (when no argument is expected)
--opt argument  (when an argument is expected)
--opt=argument  (see discussion.)

The PLLongOp struct has a boolean member named "argRequired", which is 
somewhat analogous in function to the ":" character in single letter options.

When the "argRequred" member of the longOpt is true, then either of the 
above two sntaxes for specifying an argument is required.  If no = is used,
then the next argument (in argv) will be taken as the argument for this 
option.  

When the "argRequired" member is false, then an argument may be optionally
supplied, only with the = form.  That is, --opt-argument is allowed even
when the argRequred flag is false.

Note that single-letter option strings and single-letter options work 
exactly as before, with only one exception.  When the state is created with 
a non-NULL array of PLLongOp structs, then any option starting with "--" 
is treated differently than before.

ALTERNATIVES to consider:

1. Perhaps I should have a better way to return an indicator that a long
option was parsed.

2. GNU's getopt_logn allows the long option array to specify a character 
that is returned as if it had been given as a single letter option.  
That is, one can define the keyword "opt" to act as the letter "o" and the
caller of the parser cannot tell the difference between -o and --opt from
the results.  I didn't try to do that.  Should I have done so?

ANy other thoughts or ideas?
Comment on attachment 269148 [details] [diff] [review]
another attempt - not yet sufficiently tested -v1

Alexei, while I am away, please try this patch, and 
if it works for you, please review it for NSS 3.12.
You may take this bug and revise this patch if doing
so will remove any obstacle from your development of 
the new implementation of the CERT_Verify* functions.

Note that, with this patch, both the "long" (key word)
options and the short, single letter options, are
simultaneously supported.  So the use of the long 
options is not mutually exclusive with the string of
single-character options.
Attachment #269148 - Flags: review?(alexei.volkov.bugs)
I reviewed attachment 269148 [details] [diff] [review]. Looks good to me except for a couple of points.

1) What is "--" as an option supposed to do? ("cmd -- 1st_opt -2nd_opt"). It looks like once the double dash is hit internal->endOfOpts gets set and never gets turned off.

2) In the code block starting "if (internal->minus == 2)" near the end there's the statement
                opt->value = internal->argv[++(internal->xargc)];
I believe internal->xargc == internal->argc is possible at this point which means the pointer may be past the end of the argv list.
Comment on attachment 269148 [details] [diff] [review]
another attempt - not yet sufficiently tested -v1

Cancelled review request to get this back on Nelson's radar.
Attachment #269148 - Flags: review?(alexei.volkov.bugs)
Comment on attachment 269148 [details] [diff] [review]
another attempt - not yet sufficiently tested -v1

See comment 16 for the definition of "--".
It is the same definition as in gnu's getopt_long, (or should be).

The line that you question is a copy of a pre-existing line that 
appears 25 lines farther down in the patched code.  
Do you believe that is also wrong?

Can you give a command line that will demonstrate the failure, with 
either the old code or the new code?  If so, give this patch r-.
Attachment #269148 - Flags: review?(neil.williams)
Blocks: 324744
Comment on attachment 269148 [details] [diff] [review]
another attempt - not yet sufficiently tested -v1

r+ for this patch. My 2nd claim in comment #18 is valid. See what happens with "certutil -L -d", for instance. The -d option takes a value and PL_GetNextOpt returns the next item in the argv array. Certutil catches this for other reasons but other programs might derefence the pointer--which is NULL in my test case.
Attachment #269148 - Flags: review?(neil.williams) → review+
Created Bug 389712 to track the bug mentioned in comment #21.
Comment on attachment 269148 [details] [diff] [review]
another attempt - not yet sufficiently tested -v1

Wan-Teh, I don't know how many reviews are required for checkin on the trunk of an NSPR RFE like this one.  
If no second review is required, and you don't wish to review it, please just cancel this review.  
Otherwise, please review this patch.
Thanks.
Attachment #269148 - Flags: review?(wtc)
Comment on attachment 269148 [details] [diff] [review]
another attempt - not yet sufficiently tested -v1

Thanks for the patch, Nelson.  I didn't do a thorough review.
The patch looks good.

Can you add some comments to the header file to explain how to
use the long options?

>+typedef struct PLLongOpt
>+{
>+    const char * longOptName;
>+    PRIntn       longOptValue;
>+    PRBool       argRequired;
>+} PLLongOpt;

Nit: NSPR style doesn't require padding to align the variable/member
declarations.

>     /*
>     ** If we already have a '-' in hand, xargv points to the next
>     ** option. See if we can find a match in the list of possible
>     ** options supplied.
>     */ 

The above comment needs to be updated.  Perhaps say "'-' or '--'"
instead.

>+            if (foundEqual && foundEqual[1]) 
>+            {
>+                opt->value = foundEqual + 1;
>+            }
>+            else if (longOpt->argRequired)
>+            {
>+                opt->value = internal->argv[++(internal->xargc)];
>+            }
>+            internal->xargv = &static_Nul; /* consume this */
>+            return PL_OPT_OK;

If we have "--foo= bar" and longOpt->argRequired is true, should
opt->value be NULL or "bar"?  Your code above would set opt->value
to "bar", but it would also be reasonable to set opt->value to NULL
or treat this as a "missing argument" error.  You should document
how you want to handle this case.
Attachment #269148 - Flags: review?(wtc) → review+
This patch addresses Wan-Teh's comments, and also includes a fix for
Bug 389712.
Attachment #269148 - Attachment is obsolete: true
Attachment #274573 - Flags: superreview?(wtc)
Attachment #274573 - Flags: review?(neil.williams)
Comment on attachment 274573 [details] [diff] [review]
patch with Wan-Teh's suggested changes

In re-reviewing my own previous patch, I noticed that I had inadvertently 
switched to NSS style of bracing and indentation in one function.
In this patch, I changed that to be consistent with the style used in 
the rest of this file.  

I decided that Wan-Teh's test case of "--opt= value" should return NULL
rather than "value" as the opt arg, and changed the code to do that. 
Thanks, Wan-Teh, for catching that.
Comment on attachment 274573 [details] [diff] [review]
patch with Wan-Teh's suggested changes

The comments in plgetopt.h don't explain what longOptValue is,
and that the array of PLLongOpts is terminated by a PLLongOpt
whose longOptName is NULL.

The name 'longOptValue' is confusing because the original 'value'
member means the argument of the option.  It would be nice to
harmonize the original code and new code by
changing longOptValue to longOptId, and
changing argRequired to valueRequired.

You should also explain that longOptValues need to be different.

longOptValue and longOptIndex seem to be redundant information.
Given longOptIndex, the caller can get longOptValue.

You can pass the number of elements in the longOpts array to
PL_CreateLongOptState.  This way PL_CreateLongOptState doesn't
need to count the elements in the array, and you don't need
to document that how the array should be terminated.

Would it be cleaner to design a completely independent API
for long options?
Wan-Teh, I am willing to make more changes to the header file, adding
comments, and renaming function parameters.  

The objectives of this design were:
a) to produce an extension to the existing PL_GetNextOpt family 
of NSPR functions, one that would NOT require a complete replacement
of all existing getNextOpt functions now in use in NSS programs that
want to begin to use long options.

b) to be modeled after gnu's getopt_long, as closely as possible
(without necessitating any changes to existing users of PL_GetNextOpt)
so that programmers who are already familiar with the gnu code, and 
who already use PL_GetNextOpt would be able to add the new long options 
to their existing code very quickly, without much learning curve, and
without too many mistakes.  

c) to get done quickly.  This is holding up some enhancements to certutil
which is out of option characters.  It's becoming critical path for 
libPKIX work.  

We need a solution for NSS programs ASAP.  If if becomes necessary to 
write an entirely new parsing API, then that will be done in NSS, not
NSPR. 
On Solaris the cmd/* utilities are linked with a line like:

cc -o SunOS5.10_i86pc_DBG.OBJ/addbuiltin -g ... -L../../../../dist/SunOS5.10_i86pc_DBG.OBJ/lib -lplc4 -lplds4 -lnspr4  -lthread -lnsl -lsocket -lposix4 -ldl -lc

The library where the new PL_CreateLongOptState function lives (libplc4) can be either static or dynamic both forms are built. In the command above the linker chooses the dynamic library. Attachment #269148 [details] [diff] does not add the new function to the .def file for libplc4. Are the utilities supposed to be linked statically with NSPR?
Comment on attachment 274573 [details] [diff] [review]
patch with Wan-Teh's suggested changes

r+ Perfect.
Attachment #274573 - Flags: review?(neil.williams) → review+
Neil, you privately pointed out to me that I forgot the patch to the .def file.
Thanks.  I will make another patch that will address that and also some of 
Wan-Teh's latest suggestions.  
This patch adds the missing patch to the .def file.  
The .def file patch is not needed on Windows because on Windows, all 
symbols declared with PR_IMPLEMENT() are exported whether they appear in 
the .def file or not.  That's why I failed to notice the missing .def file
patch in my own builds and testing.  

This patch adds LOTS of documentation to the header file. 
I documented the existing old functions and the new one.  

This patch makes some subtle changes to the API, as compared to the last patch.

When a long opt is not used, the value of opt->longOptIndex is now -1 instead
of zero.  This fixes the obvious ambiguity with the value zero.

The struct member formerly named longOptValue is renamed longOption.  
This signifies that it is to be used in the same manner as opt->option.

Wan-Teh observed that opt->longOption seemed redundant because that value 
can be fetched from the caller's longoptarray[opt->longOptIndex].longOption 
and that is correct.  But there is a reason for that.  It allows the caller 
to IGNORE opt->option and always look at opt->longOption.  

For single-character options, opt->longOption contains the same value as
opt->option, the value of the single character.  So, the programmer can 
choose his longOption values in his array of PLLongOpts such that the  
longOption value always uniquely identifies the option (long or short) 
that was parsed.  This relieves the programmer of the need to look first
at opt->option and then opt->longOption.   But it also gives the programmer
the opportunity to choose longOption values that intentionally match the 
values of single character options.  In this way, the programmer can 
trivially create long options that are completely equivalent to the older
single-character options.  This is a feature of GNU's getopt_long that I 
wanted to emulate in this API, and which was incorrectly emulated in the 
last patch.  Wan-Teh's comment about the apparent redundancy caused me to 
notice that this was not as intended. 

Wan-Teh, please review the new comments to see if they satisfactorily 
document the new API, and to see if the new API now seems (more) satisfactory.

We have several patches to NSS utilities that are awaiting the checkin of
this patch (or something very close to it), so timely review is much 
appreciated.
Attachment #274573 - Attachment is obsolete: true
Attachment #275085 - Flags: superreview?(wtc)
Attachment #275085 - Flags: review?(neil.williams)
Attachment #274573 - Flags: superreview?(wtc)
This is the test program I used, as a patch into nspr's test directory.
I tested it with command such as these:

getopt -a 1 -b 2 -c 3 -d 4 -e 5 -f
getopt -a 1 -b 2 -c 3 x y  -d 4 -e 5 -f
getopt -a 1 -b 2 -c --longa=11 --longb 12 --longe 3 -d 4 -e 5 -f
getopt -a 1 -b 2 -c --longa=11 --longb 12 --longe 3
getopt -a 1 -b 2 -c --longa=11 --longb 12 --longc 55 --longd 99 --longe 3
getopt -a 1 -b 2 -c --longa=11 --longb 12 -c 55 --longd 99 --longe 3
getopt -a 1 -b 2 -c --longa=11 --longb 12 -c 55 -b 99 --longe 3
getopt -a 1 -b 2 -c --longa=11 --longb 12 -c 55 -b 98 --longd 99 --longe 3

I was pleasantly surprised to see that options can be parsed even after 
positional parameters.
The output of the last command shown above is:

GOOD option: 61 ('a', index -1), argument: "1"
GOOD option: 62 ('b', index -1), argument: "2"
GOOD option: 63 ('c', index -1), argument: "(null)"
GOOD option: 61 (' ', index 0), argument: "11"
GOOD option: 62 (' ', index 1), argument: "12"
GOOD option: 63 ('c', index -1), argument: "(null)"
Positional parameter: "55"
GOOD option: 62 ('b', index -1), argument: "98"
GOOD option: 164 (' ', index 3), argument: "99"
GOOD option: 165 (' ', index 4), argument: "(null)"
Positional parameter: "3"
last result was EOL

I won't ask for review of that test program, but if someone thinks it 
should be committed and wants to review it, be my guest.
Comment on attachment 275089 [details] [diff] [review]
patch with test program for PL_CreateLongOptState

Nelson, I will review your new patch.

In your test program, optArray doesn't have the terminating element.
(In reply to comment #35)
> In your test program, optArray doesn't have the terminating element.

I was/am relying on the trailing comma, after the last substructure in the
initializer, to cause one more member to be allocated in the array. 
Since the array is static, the c language guarantees that any uninitialized
members of the array will be zero-filled.  But I am willing to explicitly
add another member to the array to make the zero termination more obvious.
Test program with more obvious array termination.
Attachment #275089 - Attachment is obsolete: true
Attachment #275208 - Flags: review?(wtc)
(In reply to comment #36)
> 
> I was/am relying on the trailing comma, after the last substructure in the
> initializer, to cause one more member to be allocated in the array. 

This doesn't work under GCC 4.0.3 and Visual C++ 2005.
I'm using VC2005
Could you compile and run this program?  Does it print 5 or 6?
Comment on attachment 275085 [details] [diff] [review]
Address more of Wan-Teh's concerns. Add missing .def file patch

r=wtc.  Good comments in plgetopt.h.

I found some minor issues, but you can check in this patch first.

Easy fixes:

>+ * Long option values (arguments) may always be given as "-name=value".

Should be "--name=value".

>+    PRIntn numLongOpts;         /* number of non-empty entries in longOpts */

This field is set but not used.  It can be removed.

In PL_CreateLongOptState, we have:

>+    opt->longOptIndex = 0;

May want to set it to -1.

>     PRIntn cop, eoo = PL_strlen(internal->options);

Move this to the "if (internal->minus)" block.

>+            opt->longOption   = longOpt->longOption  ;

Remove the extra spaces before the semicolon.

The following needs some work to verify, so you don't need to make
this change.

>+                opt->value = (++internal->xargc < internal->argc) ? 
>+                             internal->argv[internal->xargc] : NULL;

The check is not necessary because internal->argv is null-terminated
and internal->xargc is less than internal->argc before this line is
executed (ensured by the check inside the while loop at the beginning
of this function).  This line appears in both the
"if (internal->minus == 2)" and the "if (internal->minus)" blocks.

>                     internal->minus = PR_FALSE;

Change this to "internal->minus = 0;" or remove this line (it's
not necessary).  If you don't have time to verify this line can
be removed, just change PR_FALSE to 0.
Attachment #275085 - Flags: superreview?(wtc) → superreview+
Wan-Teh, you wrote:
> The check is not necessary because internal->argv is null-terminated

Since PL_CreateOptState does not create its own NULL-terminated copy of
the argv array, this statement is only true provided that the argv 
array is itself NULL-terminated.  

Can you cite any (POSIX or other) standard that specifies that argv is 
NULL terminated?   

If argv is NULL terminated, why do we ever bother checking against argc
in this code?  
The argv array has argc+1 elements: argv[0] ... argv[argc].
argv[0] is the program's name, argv[1] is the first argument,
argv[argc-1] is the last argument, and argv[argc] is a NULL
pointer.

Here are two references I found by doing a web search for "execve"
and "argc argv":
http://www.opengroup.org/onlinepubs/000095399/functions/exec.html
http://faq.cprogramming.com/cgi-bin/smartfaq.cgi?answer=1046139290&id=1043284392

I'm not saying that checking against argc is wrong.  I'm saying
that if xargc < argc, then it is correct to use argv[++xargc] in
that statement without checking again because argv[argc] is NULL.
This patch incorporates Wan-Teh's latest suggestions, and makes one
more change.  It only computes strlen(options) once, not on every call
to PL_GetNextOpt.
Attachment #275085 - Attachment is obsolete: true
Attachment #275269 - Flags: review?(wtc)
Attachment #275085 - Flags: review?(neil.williams)
Comment on attachment 275269 [details] [diff] [review]
updated patch with Wan-Teh's suggestions and more (checked in)

r=wtc.

Since you removed tabs of width 4 but added new tabs of
width 8, you should change the emacs tab-width directive
at the top of the file, which is also parsed by LXR.  Or
avoid tabs.  Please also change c-basic-offset to 4.

(We could also use a pointer to traverse the list of short
options, and avoid computing the length of the 'options'
string altogether.)
Attachment #275269 - Flags: review?(wtc) → review+
My intent was not to add any tabs, and to add only spaces.  
If I added tabs of width 8, that was a mistake that I should fix before
checkin.  What places did you find tabs of width 8 ?
Most of the rest of the lines in the files that I edited used no tabs at 
all, but only spaces.  I fixed the 3 lines on which the editor had 
inserted tabs, so that now they only use spaces, like all the lines around
them.  

Checking in include/plgetopt.h; new revision: 3.6; previous revision: 3.5
Checking in src/plc.def;        new revision: 1.7; previous revision: 1.6
Checking in src/plgetopt.c;     new revision: 3.7; previous revision: 3.6

I will await further review of the test program before committing it.
Or we can skip the test program and call this fixed.  
Attachment #275269 - Attachment description: updated patch with Wan-Teh's suggestions and more → updated patch with Wan-Teh's suggestions and more (checked in)
Blocks: 390973
Comment on attachment 275208 [details] [diff] [review]
Test program with more obvious array termination

r=wtc.

NSPR makefiles don't use $(NULL).

The test program only uses printf, so it doesn't need to include
the other system header files, especially the WIN32 and XP_UNIX
ones.

>+    {    NULL,                       },

This comma can be removed.
Attachment #275208 - Flags: review?(wtc) → review+
Checking in tests/Makefile.in; new revision: 1.17; previous revision: 1.16
Checking in tests/getopt.c;    initial revision: 1.1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: add capability to parse string command line options → add capability to parse long command line option names
Target Milestone: 4.6.3 → 4.7
You need to log in before you can comment on or make changes to this bug.