Re: [PATCH i2c-tools v3 2/2] i2ctransfer: Add optional message modifier flags

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Hi Benoît,

> @@ -202,6 +202,20 @@ static int add_flag_if_supported(__u16 *flags, unsigned long funcs, char arg)
>                         break;
>  #endif
>  
> +               case '0':
> +               case '1':
> +               case '2':
> +               case '3':
> +               case '4':
> +               case '5':
> +               case '6':
> +               case '7':
> +               case '8':
> +               case '9':
> +               case '?':
> +               case '@':
> +                       return 0;
> +
>                 default:
> 
> 
> With this we get out of the flags parsing and the "Missing direction flag"
> error message is shown. What's your opinion on this?

Basically fine with me, but we need another adaption then because this
documentation is not precise anymore:

+ * 0 if the argument is a valid and supported optional flag

Doesn't fit anymore because for the above cases, the argument is not
valid.

I think we should return a dedicated value for this 'unexpected' case.

Which brings something again to the table I was close to suggesting last
time already but back then refrained from doing so. Now, it might be
good after all:

It is about not using magic numbers for the return values of that
function but an enum with descriptive entries. Example:

enum supported_flags_retval {
	FLAGS_GOT_RW,
	FLAGS_GOT_OPTIONAL,
	FLAGS_UNEXPECTED,
	FLAGS_UNKNOWN,
};

I think this will make life for future hackers quite easier. What do you
think?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]