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 | >> PARSE_OPT_NONEG, \ >> + 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 | >> PARSE_OPT_HIDDEN, NULL, 1} > > 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 -- Regards 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
