This patch removes the need to add --help option to every program. It is added (and handled) by parse_opts().
Run tested. Please review.
Currently, usage is an array of pointers to string. It is meant to support this:
usage: foo --bar or: foo --baz or: foo -z
The patch removes this and makes usage just a string:
usage: foo --bar
Why remove this?
(1) We can achieve old format by using the usage string with many lines: usage = "foo --bar\n" " or: foo --baz\n" " or: foo -z";
(2) We don't use multi-line usage anyway.
Run tested.
Please review.
On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
Currently, usage is an array of pointers to string. It is meant to support this:
usage: foo --bar or: foo --baz or: foo -z
The patch removes this and makes usage just a string:
usage: foo --bar
Why remove this?
(1) We can achieve old format by using the usage string with many lines: usage = "foo --bar\n" " or: foo --baz\n" " or: foo -z";
(2) We don't use multi-line usage anyway.
Run tested.
Please review.
+#define OPT__VERBOSE(v) OPT_BOOL('v', "verbose", (v), "Be verbose")
Why upper 'B' in `Be verbose'?
First I don't see in any helps, second it looks bad and third I don't like it.
On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
Currently, usage is an array of pointers to string. It is meant to support this:
usage: foo --bar or: foo --baz or: foo -z
The patch removes this and makes usage just a string:
usage: foo --bar
Why remove this?
(1) We can achieve old format by using the usage string with many lines: usage = "foo --bar\n" " or: foo --baz\n" " or: foo -z";
(2) We don't use multi-line usage anyway.
Run tested.
Please review.
+#define OPT__VERBOSE(v) OPT_BOOL('v', "verbose", (v), "Be verbose")
Why upper 'B' in `Be verbose'?
First I don't see in any helps, second it looks bad and third I don't like it.
The usage text currently looks like this:
# abrtd --help usage: abrtd [options] -v, --verbose be verbose
-d Do not daemonize -s Log to syslog even with -d -t n Exit after SEC seconds of inactivity
The option descriptions should be consistent: either all start with a capital letter, or all start with a small one. Currently, they are not consistent.
So, should we make them all small or all capital?
I use capitalized description because sometimes there is more than one sentence. Random example:
-C N[:MSG] Allow only up to N connections from the same IP. New connections from this IP address are closed immediately. MSG is written to the peer before close.
having it start with a small letter and then suddenly turn into properly punctuated sentences would be weird.
On 10/08/2010 03:06 PM, Denys Vlasenko wrote:
On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
Currently, usage is an array of pointers to string. It is meant to support this:
usage: foo --bar or: foo --baz or: foo -z
The patch removes this and makes usage just a string:
usage: foo --bar
Why remove this?
(1) We can achieve old format by using the usage string with many lines: usage = "foo --bar\n" " or: foo --baz\n" " or: foo -z";
(2) We don't use multi-line usage anyway.
Run tested.
Please review.
+#define OPT__VERBOSE(v) OPT_BOOL('v', "verbose", (v), "Be verbose")
Why upper 'B' in `Be verbose'?
First I don't see in any helps, second it looks bad and third I don't like it.
The usage text currently looks like this:
# abrtd --help usage: abrtd [options] -v, --verbose be verbose
-d Do not daemonize -s Log to syslog even with -d -t n Exit after SEC seconds of inactivity
The option descriptions should be consistent: either all start with a capital letter, or all start with a small one. Currently, they are not consistent.
So, should we make them all small or all capital?
I'd like to see all small.
I use capitalized description because sometimes there is more than one sentence. Random example:
-C N[:MSG] Allow only up to N connections from the same IP. New connections from this IP address are closed immediately. MSG is written to the peer before close.
having it start with a small letter and then suddenly turn into properly punctuated sentences would be weird.
Hmm, that's true but we have always only one sentence.
On Fri, 2010-10-08 at 15:36 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 03:06 PM, Denys Vlasenko wrote:
On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
Currently, usage is an array of pointers to string. It is meant to support this:
usage: foo --bar or: foo --baz or: foo -z
The patch removes this and makes usage just a string:
usage: foo --bar
Why remove this?
(1) We can achieve old format by using the usage string with many lines: usage = "foo --bar\n" " or: foo --baz\n" " or: foo -z";
(2) We don't use multi-line usage anyway.
Run tested.
Please review.
+#define OPT__VERBOSE(v) OPT_BOOL('v', "verbose", (v), "Be verbose")
Why upper 'B' in `Be verbose'?
First I don't see in any helps, second it looks bad and third I don't like it.
The usage text currently looks like this:
# abrtd --help usage: abrtd [options] -v, --verbose be verbose
-d Do not daemonize -s Log to syslog even with -d -t n Exit after SEC seconds of inactivity
The option descriptions should be consistent: either all start with a capital letter, or all start with a small one. Currently, they are not consistent.
So, should we make them all small or all capital?
I'd like to see all small.
I have no objections. Feel free convert them all to small.
On 10/08/2010 03:36 PM, Nikola Pajkovsky wrote:
On 10/08/2010 03:06 PM, Denys Vlasenko wrote:
On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
Currently, usage is an array of pointers to string. It is meant to support this:
usage: foo --bar or: foo --baz or: foo -z
The patch removes this and makes usage just a string:
usage: foo --bar
Why remove this?
(1) We can achieve old format by using the usage string with many lines: usage = "foo --bar\n" " or: foo --baz\n" " or: foo -z";
(2) We don't use multi-line usage anyway.
Run tested.
Please review.
+#define OPT__VERBOSE(v) OPT_BOOL('v', "verbose", (v), "Be verbose")
Why upper 'B' in `Be verbose'?
First I don't see in any helps, second it looks bad and third I don't like it.
The usage text currently looks like this:
# abrtd --help usage: abrtd [options] -v, --verbose be verbose
-d Do not daemonize -s Log to syslog even with -d -t n Exit after SEC seconds of inactivity
The option descriptions should be consistent: either all start with a capital letter, or all start with a small one. Currently, they are not consistent.
So, should we make them all small or all capital?
I'd like to see all small.
I'd prefer it to start with capital (to Nikola: check the help of your beloved git..)
Jirka
I use capitalized description because sometimes there is more than one sentence. Random example:
-C N[:MSG] Allow only up to N connections from the same IP. New connections from this IP address are closed immediately. MSG is written to the peer before close.
having it start with a small letter and then suddenly turn into properly punctuated sentences would be weird.
Hmm, that's true but we have always only one sentence.
On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
Currently, usage is an array of pointers to string. It is meant to support this:
usage: foo --bar or: foo --baz or: foo -z
The patch removes this and makes usage just a string:
usage: foo --bar
Why remove this?
(1) We can achieve old format by using the usage string with many lines: usage = "foo --bar\n" " or: foo --baz\n" " or: foo -z";
(2) We don't use multi-line usage anyway.
Run tested.
Please review.
+#define OPT__VERBOSE(v) OPT_BOOL('v', "verbose", (v), "Be verbose")
Why upper 'B' in `Be verbose'?
First I don't see in any helps, second it looks bad and third I don't like it.
I committed the patch sans this capitalization change.
On 10/08/2010 04:20 PM, Denys Vlasenko wrote:
On Fri, 2010-10-08 at 14:23 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 02:10 PM, Denys Vlasenko wrote:
Currently, usage is an array of pointers to string. It is meant to support this:
usage: foo --bar or: foo --baz or: foo -z
The patch removes this and makes usage just a string:
usage: foo --bar
Why remove this?
(1) We can achieve old format by using the usage string with many lines: usage = "foo --bar\n" " or: foo --baz\n" " or: foo -z";
(2) We don't use multi-line usage anyway.
Run tested.
Please review.
+#define OPT__VERBOSE(v) OPT_BOOL('v', "verbose", (v), "Be verbose")
Why upper 'B' in `Be verbose'?
First I don't see in any helps, second it looks bad and third I don't like it.
I committed the patch sans this capitalization change.
Thank you!
This patch makes it easier to analyze "bool" type options - now they are all just bits in a single return value, no need to have an integer variable for each option.
The previous behavior is retained too: it is useful for "cumulative" options like -vvv.
The patch also removes all OPT_GROUPs.
Run tested. Please review.
On 10/08/2010 03:29 PM, Denys Vlasenko wrote:
This patch makes it easier to analyze "bool" type options - now they are all just bits in a single return value, no need to have an integer variable for each option.
The previous behavior is retained too: it is useful for "cumulative" options like -vvv.
cumulative options is done by *(int*)opt[ii].value += 1, right?
The patch also removes all OPT_GROUPs.
Run tested. Please review.
This is not wise.
+enum { + OPT_v = 1 << 0, + OPT_d = 1 << 1, + OPT_i = 1 << 2, + OPT_t = 1 << 3, + OPT_s = 1 << 4, +};
+ if (ii < sizeof(retval)*8) + retval |= (1 << ii);
this means that it's position related.
If I swap something in
static struct options abrt_action_generate_backtrace_options[] = { OPT__VERBOSE(&g_verbose), - OPT_GROUP(""), - OPT_STRING( 'd' , 0, &d_opt, "dir", "Crash dump directory"), + OPT_STRING( 'd' , 0, &dump_dir_name, "dir", "Crash dump directory"), OPT_STRING( 'i' , 0, &i_opt, "dir1[:dir2]...", "Additional debuginfo directories"), OPT_INTEGER( 't' , 0, &exec_timeout_sec, "Kill gdb if it runs for more than SECONDS"), - OPT_BOOL( 's' , 0, &s_opt, "Log to syslog even with -d"), + OPT_BOOL( 's' , 0, NULL, "Log to syslog even with -d"), OPT_END() };
it won't fit to enum.
And also I don't see any problem with that if arg is not preset then is set to 0(due to static) or is bump up by*(int*)opt[ii].value += 1
On Fri, 2010-10-08 at 16:04 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 03:29 PM, Denys Vlasenko wrote:
This patch makes it easier to analyze "bool" type options - now they are all just bits in a single return value, no need to have an integer variable for each option.
The previous behavior is retained too: it is useful for "cumulative" options like -vvv.
cumulative options is done by *(int*)opt[ii].value += 1, right?
Yes.
The patch also removes all OPT_GROUPs.
Run tested. Please review.
This is not wise.
+enum {
- OPT_v = 1 << 0,
- OPT_d = 1 << 1,
- OPT_i = 1 << 2,
- OPT_t = 1 << 3,
- OPT_s = 1 << 4,
+};
if (ii < sizeof(retval)*8)
retval |= (1 << ii);
this means that it's position related.
If I swap something in
static struct options abrt_action_generate_backtrace_options[] = { OPT__VERBOSE(&g_verbose),
- OPT_GROUP(""),
- OPT_STRING( 'd' , 0, &d_opt, "dir", "Crash dump directory"),
- OPT_STRING( 'd' , 0, &dump_dir_name, "dir", "Crash dump directory"), OPT_STRING( 'i' , 0, &i_opt, "dir1[:dir2]...", "Additional debuginfo directories"), OPT_INTEGER( 't' , 0, &exec_timeout_sec, "Kill gdb if it runs for more than SECONDS"),
- OPT_BOOL( 's' , 0, &s_opt, "Log to syslog even with -d"),
- OPT_BOOL( 's' , 0, NULL, "Log to syslog even with -d"), OPT_END() };
it won't fit to enum.
It will fit, but the value of the enum constants should be changed too, yes.
And also I don't see any problem with that if arg is not preset then is set to 0(due to static) or is bump up by*(int*)opt[ii].value += 1
I did not understand this part.
On Fri, 2010-10-08 at 14:19 +0200, Nikola Pajkovsky wrote:
On 10/08/2010 01:56 PM, Denys Vlasenko wrote:
This patch removes the need to add --help option to every program. It is added (and handled) by parse_opts().
Run tested. Please review.
apply, thanks
Applied with small changes. Thanks.
crash-catcher@lists.fedorahosted.org