Hi, I would want to integrate this into 3.4, since I've opened it. Could one of you please prepare a combined patch, that contains both Gergely's and Jakub's work? Thanks. On Wed, 2011-04-13 at 16:13 +0200, Gergely Nagy wrote:
Jakub Jankowski <shasta@toxcorp.com> writes:
Tuesday 12 of April 2011 10:50:05 Jakub Jankowski napisał(a):
Saturday 09 of April 2011 13:05:38 Gergely Nagy napisał(a):
This implements the $(substr) template function - a fairly primitive one: it does error checking, but there's no error reporting present, therefore invalid function calls will just get ignored.
Thanks! I'll be testing this later this week and report back issues (if any).
This is what I came up with, based on your patch: - it changes arguments order to $(substr <string> <offsetstart> [length]) - so string is first - offset = 0 means beginning of the string (and not 1) - called without third (length) argument returns substring from desired start offset to the end of the string - works with negative offsets/lengths in a manner similar to other implementations: - negative offset means "start that many characters counting from the end of the string", with -1 being the last character - negative length means "return substring from desired start up to that many characters from the end of the string" - adds some msg_error()s on argument parsing failures
All of these sound good and reasonable, thank you!
I'm not entirely sure if there are no off-by-one (or even off-by-many :) errors, I'd be grateful if someone could check that. I also had issues comparing negative glongs (signed) with GString's structure len field, which is gsize (so unsigned), hence that ugly type change at the beginning. Can anyone point me to a *right* direction there?
I think you're on the right track there, but see my comments on the patch itself.
+static void +tf_substr(LogMessage *msg, gint argc, GString *argv[], GString *result) +{ + glong start, len; + glong argv0len; + + /* + * We need to cast argv[0]->len, which is gsize (so unsigned) type, to a + * signed type, so we can compare negative offsets/lengths with string + * length. But if argv[0]->len is bigger than G_MAXLONG, this can lead to + * completely wrong calculations, so we'll just return nothing (alternative + * would be to return original string and perhaps print an error...) + */ + if (argv[0]->len >= G_MAXLONG) { + /* msg_error("$(substr) error: string length is too big to fit into glong type", NULL); */ + return; + } + /* K&R probably hate me */ + argv0len = argv[0]->len; +
The intent here is good, but it could - in my opinion - be made a little more efficient:
if (argv[0]->len >= G_MAXLONG) { msg_error("$(substr) error: string is too long", NULL); return; }
And then just cast argv[0]->len to glong if so need be (see later).
I'd say the error message can be simple too, mentioning the exact type is in my opinion, not needed. For those who'd understand what it means, it doesn't add much extra value, as they can just look at the source. For others, it's confusing.
Short & to the point.
Also, if we return due to a string being too long, I believe we should print at least a warning (but an error seems more appropriate). Returning the original string in this case sounds non-intuitive, so - as you did in your patch - it's better to do nothing instead.
+ /* if we were called with >2 arguments, third was desired length */ + if (argc > 2) { + if (!tf_substr_parse_int(argv[2]->str, &len)) { + msg_error("$(substr) parsing failed, length could not be parsed", evt_tag_str("length", argv[2]->str), NULL); + return; + } + } else + len = argv0len;
len = (glong)argv[0]->len
And then you saved a variable, accomplished the same thing, and noone hates you! :]
+ /* + * if requested substring length is negative and beyond string start, do nothing; + * also if it is greater than string length, limit it to string length + */ + if (len < 0 && -(len) > argv0len) + return; + else if (len > argv0len) + len = argv0len;
See above. You can cast argv[0]->len to glong within the if, to avoid compiler warnings - and it's safe too, since you checked the length at the top.
This goes for the rest of the len<->argv0len stuff too. However, explicit casting is merely my preference, using a variable is perfectly good too. Just name it something different then, like... "stringlen" - so one reading the code will have an idea what it is, without having to scroll up and check how it is initialized.
Other than these, your patch looks fine on the first read, good work!
I'll have another look at it as soon as possible, and apply it to my 3.3 branch too.
-- Bazsi