On Wed, Mar 12, 2014 at 6:47 PM, Duy Nguyen <pclo...@gmail.com> wrote:
> By convention, no full stop in the subject line. The subject should
> summarize your changes and "add ..NONEG" is just one part of it. The
> other is "convert to use ...NONEG". So I suggest "parse-options:
> convert to use new macro OPT_SET_INT_NONEG()" or something like that.
> You should also explain in the message body (before Signed-off-by:)
> why this is a good thing to do. My guess is better readability and
> harder to make mistakes in the future when you have to declare new
> options with noneg.

Thanks for pointing that out, I'll do as you suggested.

> On Tue, Mar 11, 2014 at 5:50 PM, Yuxuan Shui <yshu...@gmail.com> wrote:
>> Reference: http://git.github.io/SoC-2014-Microprojects.html
> I think this project is actually two: one is convert current
> {OPTION_SET_INT, ... _NONEG} to the new macro, which is truly a micro
> project. The other is to find OPT_...(..) that should have NONEG but
> does not. This one may need more time because you need to check what
> those options do and if it makes sense to have --no- form.

Hmm, this microproject has been striked from the ideas page, maybe I
should switch to another one...

> I think we can focus on the {OPTION_..., _NONEG} conversion, which
> should be enough get you familiar with git community.
>> diff --git a/parse-options.h b/parse-options.h
>> index d670cb9..7d20cf9 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -125,6 +125,10 @@ struct option {
>>                                       (h), PARSE_OPT_NOARG }
>>  #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
>>                                       (h), PARSE_OPT_NOARG, NULL, (i) }
>> +#define OPT_SET_INT_NONEG(s, l, v, h, i)  \
>> +                                     { OPTION_SET_INT, (s), (l), (v), NULL, 
>> \
>> +                                     (h), PARSE_OPT_NOARG | 
>> +                                     NULL, (i) }
>>  #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
>>  #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
>>                                       (h), PARSE_OPT_NOARG | 
> To avoid the proliferation of similar macros in future, I think we
> should make a macro that takes any flags, e.g.
> #define OPT_SET_INT_X(s, l, v, h, i, flags) {  ....., PARSE_OPT_NOARG
> | PARSE_OPT_ ## flags, NULL, (i) }
> and we can use it for NONEG like "OPT_SET_INT_X(...., NONEG)". We
> could even redefine OPT_SET_INT() to use OPT_SET_INT_X() to reduce
> duplication.

I could do that, but it seems only the NONEG flag is used in the code.

> While we're at NONEG, I see that builtin/grep.c has this construct "{
> OPTION_INTEGER...NONEG}" and builtin/read-tree.c has "{
> OPTION_STRING..NONEG}". It would be great if you could look at them
> and see if NONEG is really needed there, or simpler forms
> OPT_INTEGER(...) and OPT_STRING(...) are enough.

I've grep'd through the source code, and most of the manually filled
option structures don't seems to have a pattern. And I think writing a
overly generalized macro won't help much.

> You might need to read parse-options.c to understand these options.
> Documentation/technical/api-parse-options.txt should give you a good
> overview.
> You could also think if we could transform "{ OPTION_CALLBACK.... }"
> to OPT_CALLBACK(...). But if you do and decide to do it, please make
> it a separate patch (one patch deals with one thing).
> That remaining of your patch looks good.

Thanks for reviewing my patch.

> --
> Duy

Yuxuan Shui
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to