On Thu, Apr 15, 2021 at 06:47:29PM +0200, Tim Düsterhus wrote:
> You know that I'm obsessed with the correct use of data types, so please
> find three CLEANUP patches attached. Feel free to drop the ones you don't
> agree with :-)
I don't disagree with them, but am just having a few comments on style
in turn :-)
> static int sample_conv_json_query(const struct arg *args, struct sample
> *smp, void *private)
> {
> struct buffer *trash = get_trash_chunk();
> - const char *p; /* holds the temporary string from mjson_find */
> - int tok, n; /* holds the token enum and the length of the value */
> - int rc; /* holds the return code from mjson_get_string */
> + const char *token; /* holds the temporary string from mjson_find */
> + int token_size; /* holds the length of <token> */
>
> - tok = mjson_find(smp->data.u.str.area, smp->data.u.str.data,
> args[0].data.str.area, &p, &n);
> + enum mjson_tok token_type = mjson_find(smp->data.u.str.area,
> smp->data.u.str.data, args[0].data.str.area, &token, &token_size);
As much as possible, please try to avoid assignments in declarations,
especially when using functions. There are several reasons that make
this annoying:
- sometimes just changing one arg requires to reorder the declarations
- debugging with them is painful
- trying to comment some of them out is painful as well
- it discourages from adding finer control (e.g. if you need to do
this or that using and if/else block, you also have to significantly
modify them).
- quite a few times I've seen code like this been deleted because when
the compiler says "unused variable foo", it makes one think it's now
OK to delete it, except that the function possibly had a desired side
effect
=> thus better declare the enum with the variables
Same goes for one or two other ones in the switch/case statements.
I'm otherwise OK.
Thanks!
Willy
