Willy, On 4/16/21 8:24 AM, Willy Tarreau wrote:
- 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 forgot to do in this patch, but I prefer directly assigning, because I can mark the variables `const` that way. It also increases verbosity for these single-assignment variables which IMO reduces readability.
Also I really dislike the (old) C style of declaring all variables at the very top of the scope. Most notably not being allowed to `for (size_t i = 0; ...; ...)` is bothering me quite a bit.
But who am I to complain? Updated patches attached. I left the 'trash' one as-is, because that one really is a common pattern. I hope I did not miss anything else :-)
Best regards Tim Düsterhus
>From d37a4788b6be31a7dd53a0724baaaeb4e160b85d Mon Sep 17 00:00:00 2001 From: Tim Duesterhus <[email protected]> Date: Thu, 15 Apr 2021 18:08:48 +0200 Subject: [PATCH v2 1/3] CLEANUP: sample: Improve local variables in sample_conv_json_query To: [email protected] Cc: [email protected] This improves the use of local variables in sample_conv_json_query: - Use the enum type for the return value of `mjson_find`. - Do not use single letter variables. - Reduce the scope of variables that are only needed in a single branch. - Add missing newlines after variable declaration. --- src/sample.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/sample.c b/src/sample.c index 728c7c76d..c2d9beda3 100644 --- a/src/sample.c +++ b/src/sample.c @@ -3723,16 +3723,17 @@ static int sample_check_json_query(struct arg *arg, struct sample_conv *conv, 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; - switch(tok) { + token_type = mjson_find(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, &token, &token_size); + + switch (token_type) { case MJSON_TOK_NUMBER: if (args[1].type == ARGT_SINT) { - smp->data.u.sint = atoll(p); + smp->data.u.sint = atoll(token); if (smp->data.u.sint < JSON_INT_MIN || smp->data.u.sint > JSON_INT_MAX) return 0; @@ -3740,6 +3741,7 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo smp->data.type = SMP_T_SINT; } else { double double_val; + if (mjson_get_number(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, &double_val) == 0) { return 0; } else { @@ -3757,17 +3759,21 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo smp->data.type = SMP_T_BOOL; smp->data.u.sint = 0; break; - case MJSON_TOK_STRING: - rc = mjson_get_string(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, trash->area, trash->size); - if (rc == -1) { + case MJSON_TOK_STRING: { + int len; + + len = mjson_get_string(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, trash->area, trash->size); + + if (len == -1) { /* invalid string */ return 0; } else { - trash->data = rc; + trash->data = len; smp->data.u.str = *trash; smp->data.type = SMP_T_STR; } break; + } default: /* no valid token found */ return 0; -- 2.31.1
>From ecc5b3f91d8fc02f1cd5ed84949cb0aef02df075 Mon Sep 17 00:00:00 2001 From: Tim Duesterhus <[email protected]> Date: Thu, 15 Apr 2021 18:14:32 +0200 Subject: [PATCH v2 2/3] CLEANUP: sample: Explicitly handle all possible enum values from mjson To: [email protected] Cc: [email protected] This makes it easier to find bugs, because -Wswitch can help us. --- src/sample.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/sample.c b/src/sample.c index c2d9beda3..89c443757 100644 --- a/src/sample.c +++ b/src/sample.c @@ -3774,8 +3774,19 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo } break; } - default: - /* no valid token found */ + case MJSON_TOK_NULL: + case MJSON_TOK_ARRAY: + case MJSON_TOK_OBJECT: + /* We cannot handle these. */ + return 0; + case MJSON_TOK_INVALID: + /* Nothing matches the query. */ + return 0; + case MJSON_TOK_KEY: + /* This is not a valid return value according to the + * mjson documentation, but we handle it to benefit + * from '-Wswitch'. + */ return 0; } return 1; -- 2.31.1
>From c1ed131af19ac5f6aa1a4172c5a837b406eafd6e Mon Sep 17 00:00:00 2001 From: Tim Duesterhus <[email protected]> Date: Thu, 15 Apr 2021 18:40:06 +0200 Subject: [PATCH v2 3/3] CLEANUP: sample: Use explicit return for successful `json_query`s To: [email protected] Cc: [email protected] Move the `return 1` into each of the cases, instead of relying on the single `return 1` at the bottom of the function. --- src/sample.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/sample.c b/src/sample.c index 89c443757..ed6063168 100644 --- a/src/sample.c +++ b/src/sample.c @@ -3739,26 +3739,30 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo return 0; smp->data.type = SMP_T_SINT; + + return 1; } else { double double_val; - if (mjson_get_number(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, &double_val) == 0) { + if (mjson_get_number(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, &double_val) == 0) return 0; - } else { - trash->data = snprintf(trash->area,trash->size,"%g",double_val); - smp->data.u.str = *trash; - smp->data.type = SMP_T_STR; - } + + trash->data = snprintf(trash->area,trash->size,"%g",double_val); + smp->data.u.str = *trash; + smp->data.type = SMP_T_STR; + + return 1; } - break; case MJSON_TOK_TRUE: smp->data.type = SMP_T_BOOL; smp->data.u.sint = 1; - break; + + return 1; case MJSON_TOK_FALSE: smp->data.type = SMP_T_BOOL; smp->data.u.sint = 0; - break; + + return 1; case MJSON_TOK_STRING: { int len; @@ -3767,12 +3771,13 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo if (len == -1) { /* invalid string */ return 0; - } else { - trash->data = len; - smp->data.u.str = *trash; - smp->data.type = SMP_T_STR; } - break; + + trash->data = len; + smp->data.u.str = *trash; + smp->data.type = SMP_T_STR; + + return 1; } case MJSON_TOK_NULL: case MJSON_TOK_ARRAY: @@ -3789,7 +3794,9 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo */ return 0; } - return 1; + + my_unreachable(); + return 0; } -- 2.31.1
