Hi,
Thank you Tim and Willy for your quick and detailed feedback. I'll continue
working on the patch at the earliest convenience.
In the meantime, I'd like to further discuss some points:
- The square brackets in the converter "signature" in configuration.txt are
> mismatched.
> - In the explanation there should not be square brackets around [<delim>].
Actually, there are no square brackets around <delim>:
+add_item(<delim>,[<var>][,<suff>]])
but I definitely wasn't sure how to define it since both <var> and <suff>
arguments are optional but at least one has to be specified. That's because
a minimum number of arguments is set to 2 as less than that wouldn't make
sense. The way the parser works the following configs work(no parser
errors):
add_item(",",), add_item(",",,suff), add_item(",",,)
but ofc. only in the second case <delim> and <suff> are added to the input
and in the first and third cases nothing is added as both <var> and <suff>
are "empty".
This leads us to the next point :
- The documentation states "It(<suff>) may also be omitted but only if the
> <var> is supplied". You should verify this with a 'check' function (you're
> currently reusing smp_check_concat)
Since the minimum number of arguments is set to 2, maybe there is no need
to verify it because if both <suff> and <var> are omitted the parser will
complain about missing arguments. So maybe the best solution would be to
remove the "but only if the <var> is supplied" part from the documentation
and still use the smp_check_concat check. Let me know how this sounds to
you.
Please place it at the top to fix the alphabetical order, and feel free
> to move concat with it as it was obviously misplaced.
Sure, but I guess that would require a separate patch(the concat part) to
keep this one clean and focused on the new feature?
Also, apologies for the cosmetic issues, I will try to take better care.
Best regards,
Nikola
On Fri, 1 Apr 2022 at 08:47, Willy Tarreau <w...@1wt.eu> wrote:
> Nikola,
>
> thanks for your contribution. I agree with Tim's points, and that's a
> good indicator of the quality of your patch, since all of this is purely
> cosmetic :-)
>
> I do have one minor cosmetic comment to add here:
>
> +static int sample_conv_add_item(const struct arg *arg_p, struct sample
> *smp, void *private)
> +{
> + struct buffer *trash;
> + struct sample tmp;
> + int max, v_flag;
> +
> + trash = alloc_trash_chunk();
> + if (!trash)
> + return 0;
>
> Please use a different name from "trash" for this dynamic trash. Call
> it "tmpbuf", "trash2" or whatever you see fit. We've done this mistake
> a few times in the past to call such dynamic buffers "trash" but the
> problem is that it causes confusion when reading the code because
> "trash" is a well-known thread-local global variable and it's easy to
> confuse one with the other. Actually the other places where the "trash"
> name is used like this happened because a conflict forced us to dyanmically
> allocate a trash to replace the current one.
>
> { "regsub", sample_conv_regsub, ARG3(2,REG,STR,STR),
> sample_conv_regsub_check, SMP_T_STR, SMP_T_STR },
> { "sha1", sample_conv_sha1, 0,
> NULL, SMP_T_BIN, SMP_T_BIN },
> { "concat", sample_conv_concat, ARG3(1,STR,STR,STR),
> smp_check_concat, SMP_T_STR, SMP_T_STR },
> + { "add_item",sample_conv_add_item, ARG3(2,STR,STR,STR),
> smp_check_concat, SMP_T_STR, SMP_T_STR },
> { "strcmp", sample_conv_strcmp, ARG1(1,STR),
> smp_check_strcmp, SMP_T_STR, SMP_T_SINT },
>
> Please place it at the top to fix the alphabetical order, and feel free
> to move concat with it as it was obviously misplaced.
>
> Other than that, it looks pretty good, thank you very much for taking
> care of this useful feature!
>
> Willy
>
