Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
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.
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.
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.
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.
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.
--
Duy
--
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
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
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
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
Duy Nguyen pclo...@gmail.com writes: 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. As I said elsewhere, I am not sure if doubling the number of OPT_type macros as your micro suggestion proposes is the right solution to the problem. What are we trying to solve? -- 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
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
Hi, On Thu, Mar 13, 2014 at 2:30 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: 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. As I said elsewhere, I am not sure if doubling the number of OPT_type macros as your micro suggestion proposes is the right solution to the problem. What are we trying to solve? OK, I'll switch to another micro project. -- 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
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
Yuxuan Shui yshu...@gmail.com writes: On Thu, Mar 13, 2014 at 2:30 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: 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. As I said elsewhere, I am not sure if doubling the number of OPT_type macros as your micro suggestion proposes is the right solution to the problem. What are we trying to solve? OK, I'll switch to another micro project. That is fine, but note that it was not an objection but was a pure question. I was asking you to explain why this is a good change. -- 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
