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@xxxxxxxxx> wrote:
>
> Paul Tan <pyokagan@xxxxxxxxx> writes:
>
>> On Sun, Mar 29, 2015 at 4:32 PM, Ivan Ukhov <ivan.ukhov@xxxxxxxxx> 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.
>> */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html