Re: [PATCH] parse-options: fix the description of defval

Paul Tan [off-list ref] writes:

On Sun, Mar 29, 2015 at 4:32 PM, Ivan Ukhov [off-list ref] wrote:

quoted

Since the deletion of OPT_SET_PTR, defval can no longer contain a pointer.

Actually, it can contain a pointer for OPTION_CMDMODE, OPTION_STRING
and OPTION_FILENAME. Since we are on the topic of updating the
documentation, I think it would be great if the documentation
mentioned these option types as well.

Actually, both of you are correct ;-)

The patch text you are responding to is not saying anything wrong.
The only thing Ivan stated incorrectly is the log message.

    parse-options.h: OPTION_{BIT,SET_INT} do not store pointer to defval

    When 20d1c652 (parse-options: remove unused OPT_SET_PTR,
    2014-03-30) removed OPT_SET_PTR, the comment in the header that
    describes what the option did to defval field was left behind by
    mistake.  Remove it.

or something, perhaps?

It is a different issue if we want to describe uses of `defval` by
all other macros like OPTION_STRING.  We should make it easier for
our contributors (me included) to find how each option macros can be
used, and how OPTION_XYZ uses defval must be described somewhere,
but I personally think bloating the description of `defval` is not a
good way to do so.  Description of OPTION_XYZ may be the first place
for programmers to go to find how it should be used, so perhaps it
is a better idea to enrich descriptions there instead of here.

In other words, it may be an improvement to say only the general
principle shared across all uses e.g. "default value to fill .value
with", without mentioning specifics of exceptions (e.g. "for
OPTION_BIT it is not even a default, it is _the_ value") in this
section.  Instead, comment OPTION_BIT with "the defval field is used
to store the bitmask used to set/clear/flip" or something.

But as I said, that is a different issue.

Hello Junio,


Yes, actually my intention to fix that comment was solely based on its content. I saw that the elements in the first set, {BIT,SET_INT}, did not match the elements in the second, {mask,integer,pointer}. Then I found that commit removing OPT_SET_PTR, and “pointer” seemed to be a leftover, which I decided to eliminate. My commit message was saying something different, I should admit)

I totally agree with your about mentioning only the general principle and leaving details to particular macros.


Regards,
Ivan

On Mar 29, 2015, at 7:39 PM, Junio C Hamano [off-list ref] wrote:

Paul Tan [off-list ref] writes:

quoted

On Sun, Mar 29, 2015 at 4:32 PM, Ivan Ukhov [off-list ref] wrote:

quoted

Since the deletion of OPT_SET_PTR, defval can no longer contain a pointer.

Actually, it can contain a pointer for OPTION_CMDMODE, OPTION_STRING
and OPTION_FILENAME. Since we are on the topic of updating the
documentation, I think it would be great if the documentation
mentioned these option types as well.

Actually, both of you are correct ;-)

The patch text you are responding to is not saying anything wrong.
The only thing Ivan stated incorrectly is the log message.

   parse-options.h: OPTION_{BIT,SET_INT} do not store pointer to defval

   When 20d1c652 (parse-options: remove unused OPT_SET_PTR,
   2014-03-30) removed OPT_SET_PTR, the comment in the header that
   describes what the option did to defval field was left behind by
   mistake.  Remove it.

or something, perhaps?

It is a different issue if we want to describe uses of `defval` by
all other macros like OPTION_STRING.  We should make it easier for
our contributors (me included) to find how each option macros can be
used, and how OPTION_XYZ uses defval must be described somewhere,
but I personally think bloating the description of `defval` is not a
good way to do so.  Description of OPTION_XYZ may be the first place
for programmers to go to find how it should be used, so perhaps it
is a better idea to enrich descriptions there instead of here.

In other words, it may be an improvement to say only the general
principle shared across all uses e.g. "default value to fill .value
with", without mentioning specifics of exceptions (e.g. "for
OPTION_BIT it is not even a default, it is _the_ value") in this
section.  Instead, comment OPTION_BIT with "the defval field is used
to store the bitmask used to set/clear/flip" or something.

But as I said, that is a different issue.

quoted

diff --git a/parse-options.h b/parse-options.h
index 7940bc7..c71e9da 100644
--- a/parse-options.h
+++ b/parse-options.h

@@ -95,8 +95,7 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
 *
 * `defval`::
 *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
- *   OPTION_{BIT,SET_INT} store the {mask,integer,pointer} to put in
- *   the value when met.
+ *   OPTION_{BIT,SET_INT} store the {mask,integer} to put in the value when met.
 *   CALLBACKS can use it like they want.
 */