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

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

[Ivan Ukhov]

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

Signed-off-by: Ivan Ukhov <ivan.ukhov@gmail.com>
---
 parse-options.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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.
  */
 struct option {
--
1.8.4

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

[Paul Tan]

Hi,

On Sun, Mar 29, 2015 at 4:32 PM, Ivan Ukhov <ivan.ukhov@gmail.com> wrote:
> 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.

Thanks,
Paul

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

[Ivan Ukhov]

Hello Paul,


> On Mar 29, 2015, at 11:08 AM, Paul Tan <pyokagan@gmail.com> wrote:
> 
> Actually, it can contain a pointer for OPTION_CMDMODE, OPTION_STRING
> and OPTION_FILENAME.

I have checked the definitions of the three macros you mentioned, and it seems that none of them uses defval to store pointers. OPTION_CMDMODE stores chars and integers. OPTION_STRING does not use defval at all (pointers go in a different field of the option struct), and the same applies to OPTION_FILENAME. I am probably missing something; can you please clarify what you mean? Thank you.


Regards,
Ivan

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

[Paul Tan]

Hi,

On Sun, Mar 29, 2015 at 5:28 PM, Ivan Ukhov <ivan.ukhov@gmail.com> wrote:
> I have checked the definitions of the three macros you mentioned, and it seems that none of them uses defval to store pointers. OPTION_CMDMODE stores chars and integers. OPTION_STRING does not use defval at all (pointers go in a different field of the option struct), and the same applies to OPTION_FILENAME. I am probably missing something; can you please clarify what you mean? Thank you.

For OPTION_STRING, if the PARSE_OPT_OPTARG flag is set (as the
documentation already states), the option can be provided on the
command line without any corresponding argument. If provided as so,
the string pointer of defval is used.

See get_value() in parse-options.c

I haven't used the other option types before yet (just did a grep
'defval' in parse-options.c) so I don't know what they do with defval.
That's why it would be nice if they were documented :-)

Regards,
Paul

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

[Ivan Ukhov]

Hello Paul,


Yes, you are right. Thank you for the clarification!


Regards,
Ivan

> On Mar 29, 2015, at 3:27 PM, Paul Tan <pyokagan@gmail.com> wrote:
> 
> Hi,
> 
> On Sun, Mar 29, 2015 at 5:28 PM, Ivan Ukhov <ivan.ukhov@gmail.com> wrote:
>> I have checked the definitions of the three macros you mentioned, and it seems that none of them uses defval to store pointers. OPTION_CMDMODE stores chars and integers. OPTION_STRING does not use defval at all (pointers go in a different field of the option struct), and the same applies to OPTION_FILENAME. I am probably missing something; can you please clarify what you mean? Thank you.
> 
> For OPTION_STRING, if the PARSE_OPT_OPTARG flag is set (as the
> documentation already states), the option can be provided on the
> command line without any corresponding argument. If provided as so,
> the string pointer of defval is used.
> 
> See get_value() in parse-options.c
> 
> I haven't used the other option types before yet (just did a grep
> 'defval' in parse-options.c) so I don't know what they do with defval.
> That's why it would be nice if they were documented :-)
> 
> Regards,
> Paul

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

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> On Sun, Mar 29, 2015 at 4:32 PM, Ivan Ukhov <ivan.ukhov@gmail.com> wrote:
>> 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.

> 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.
>   */

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

[Ivan Ukhov]

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 <gitster@pobox.com> wrote:
> 
> Paul Tan <pyokagan@gmail.com> writes:
> 
>> On Sun, Mar 29, 2015 at 4:32 PM, Ivan Ukhov <ivan.ukhov@gmail.com> wrote:
>>> 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.
> 
>> 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.
>>  */

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

[Junio C Hamano]

Ivan Ukhov <ivan.ukhov@gmail.com> writes:

> 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

OK, lest we forget, please send an updated patch marked as v2 with
proposed commit log message that does not say something different,
then ;-)

Thanks.