On Wed, Apr 26, 2017 at 3:02 PM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote:
> On 4/26/17 2:30 AM, Jan Tulak wrote:
>> On Tue, Apr 25, 2017 at 6:52 PM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote:
>>>
>>> i.e. I'd argue that a bare boolean flag specification should /always/
>>> enable the feature. At that point, you can say within your code
>>> "if min = 0 and max = 1, this is a boolean, and the flag may be specified
>>> without a value, in which case the value is set to 1, and the feature
>>> is enabled."
>>>
>>> At that point you don't need this extra structure member, and you won't
>>> have to re-state it 56 times, thus simplifying the code.
>>>
>>> What do you think? I'd happily review and merge 2 patches, which:
>>>
>>> 1) Makes rmapbt and reflink behave like every other boolean flag, and
>>>
>> This sounds reasonable.
>>
>>> 2) Removes "defaultval" altogether, and uses min=0/max=1 as a sign that
>>> the option is a boolean, with a bare flag name allowed to enable it.
>>>
>> I wonder if a .is_flag = true (and no other min/max/defaultval...)
>> wouldn't be better solution. the min=0/max=1 detection is a bit
>> implicit and may not be apparent in the future. If we limit flags to
>> 0/1 values, then defaultval can be removed either way, but it is clear
>> on the first glance what the option does.
>
> Well, yes, it is implicit - but an option with a minimum 0 and maximum 1
> can be interpreted no other way, IMHO. You could document it as such
> for clarity. But I can see the value in a single "is_flag" marker
> I suppose.
>
> If you want to start typing options, you may want to consider also
> adding .is_string, and then check the option against these
> rules in getnum and getstr - i.e. if is_flag or is_string are set,
> then minval, maxval, is_power_2 must not not set, etc, so the
> developer can't get it wrong.
>
> We'd end up with something like this:
>
> { .index = D_FILE,
> .conflicts = { LAST_CONFLICT },
> .is_flag = 1,
> },
> { .index = D_NAME,
> .conflicts = { LAST_CONFLICT },
> .is_string = true,
> },
>
> which seems reasonable to me.
>
Well, the actual code will be something like this:
{ .index = D_FILE,
.conflicts = { LAST_CONFLICT },
.type = BOOL,
},
{ .index = D_NAME,
.conflicts = { LAST_CONFLICT },
.type = STRING,
},
because I already have patches that adds type (if you remember the
union stuff from my last big set). I just didn't realised I have this
code just waiting for some rebase and fixes when I wrote the reply.
So for now, I will keep this as it is and once the type is introduced,
the defaultval can be removed.
Cheers,
Jan
> Thanks,
> -Eric
--
Jan Tulak
jtulak@xxxxxxxxxx / jan@xxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
