Hi Willy, Thanks for the review.
On Tue, Jan 5, 2021 at 12:27 PM Willy Tarreau <[email protected]> wrote: > > +/* Check url-encode type */ > > +enum encode_type { > > + ENC_QUERY = 0, > > +}; > > Please leave a blank line here between the end of the enum and the function. fixed in v3. I don't know why but I wrongly copied a similar style in sample.c (input_type) > As much as I remember, check functions are never called with a NULL args > list, it's always an array, so you can drop this test. true, fixed in v3. I will fix what I saw in sample.c in another patch (json_check). > > + if (arg->type != ARGT_STR) { > > + memprintf(err, "Unexpected arg type for encoding"); > > + return 0; > > + } > > The expression parser guarantees that you can't have anything else, because > you mentioned your converter takes a mandatory string argument. Thus this > test can be removed as well. understood fixed in v3. > > + > > + if (arg && arg->type == ARGT_STR) { > > This test is redundant with to the two previous ones. And since we know > they can never happen, you can simply remove the test and leave the code > below out of the block. arg I should have caught this one before. thanks. > > + for (i = 0; i < 256; i++) { > > + if (!((i >= 'a' && i <= 'z') || (i >= 'A' && i <= 'Z') > > + || (i >= '0' && i <= '9') || > > + i == '-' || i == '.' || i == '_' || i == '~')) > > + ha_bit_set(i, url_encode_map); > > + } > > No, this is not a good idea, you're doing this in the stack for every > single call. Better put it outside, as a global variable, and initalize > it in an initcall function. I'm even seeing that we have similarly named > variables in log.c, maybe you can call your variable() > or url_enc_query_map() or anything else that makes sense to you. very good point indeed. fixed in v3 > > + } else { > > + return 0; > > + } > > + ret = encode_string(trash->area, trash->area + trash->size, '%', > > + url_encode_map, smp->data.u.str.area); > > + if (ret == NULL || *ret != '\0') > > + return 0; > > + trash->data = strlen(trash->area); > > From what I'm seeing from the encode_string() function, it returns the > pointer to the trailing zero so that you don't have to run a costly > strlen() over it, thus you can simply write: > > trash->data = ret - trash->area; fixed -- William
