On Wed, Apr 26, 2017 at 10:21 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> On Tue, Apr 25, 2017 at 09:45:53AM +0200, Jan Tulak wrote:
>> On Tue, Apr 25, 2017 at 5:04 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
>> > On Sun, Apr 23, 2017 at 08:54:55PM +0200, Jan Tulak wrote:
>> >> opts[MAX_OPTS] = {
>> >> +#define OPT_B 0
>> >
>> > I find these defines blinding on reading the struct declaration, we could
>> > instead just stuff them into an enum, which would also enable us to shift all
>> > the documentation into the header of the enum using whatever doc format we use.
>>
>> The grouping is here to keep the style as it is with suboptions
>> index/string. However, all these defines (OPT_X and X_FOO) are
>> separated and moved into a single place later on in one patch anyway.
>> I just didn't include that patch in this set as I had to make the line
>> somewhere if I want to make multiple smaller and easier to review and
>> merge sets rather than the one big monster as before.
>
> Makes sense.
>
>> There is one point I'm not sure about, though. We have to restart the
>> counting for suboptions, like this:
>>
>> enum subopts {
>> B_size = 0,
>> B_LOG,
>> D_AGCOUNT = 0,
>> D_...,
>> L_xxx = 0,
>> L_...,
>> }
>>
>> I didn't found anywhere if it is a good practice or not. But again, I
>> just submitted it as it was with a note in my TODOs to find out more
>> out it and update the patch changing it later on.
>
> Ugh yeah no that would be pretty terrible, how about something like:
>
> enum subopt_zero_idx {
> SUBOPT_ZERO_A = 0,
> SUBOPT_ZERO_B,
> SUBOPT_ZERO_C,
> };
>
> enum subopt_one_idx {
> SUBOPT_ONE_A = 0,
> SUBOPT_ONE_B,
> };
>
> enum subopt_two_idx {
> SUBOPT_TWO_A = 0,
> };
>
> union subopt_idx {
> enum subopt_zero_idx subopt_zero;
> enum subopt_one_idx subopt_one;
> enum subopt_two_idx subopt_two;
> };
>
> Demo: http://drvbp1.linux-foundation.org/~mcgrof/demos/demo-flexible-array-subopts.c
>
> Luis
Ah, unions didn't cross my mind for this. And the demo is nice. I keep
the #defines in this patch, but this definitely deserves its patch or
two too.
Cheers,
Jan
--
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
