I'll make my commit messages more detailed.
The strdup() is necessary. Due to the string/integer duality of the
values, integer values must be coerced into strings on occasion. This
is done with the valstr() function which will print the integer into
the buffer given. In this case that buffer is local to the match()
function.
If vstr contained a string, then sure we could just return (Val){ s, 0
}; and everything is fine. But if vstr contained an integer, it is
changed to a string stored in buf1 and s points into buf1. In this
case if we did as above, the pointer returned points into buf1 which
has local storage (colloquially "on the stack"). After match() returns
this pointer would be invalid.
I have yet to think of a good way to avoid this problem, if one comes
to mind please let me know and/or just go fix it. If we do keep the
strdup() and want to be able to free() it then I'd probably just
create a global list/stack/something and add allocations to it so they
can be free()d after successful execution.
Ok, while writing this email I went ahead and did that, the diff is
attached. If you think it's worth the extra code for correctness go
ahead and commit it. It's not too much code, but it's also a small
problem. I'm torn...
-emg
On Thu, Feb 26, 2015 at 12:14 PM, Dimitris Papastamos <s...@2f30.org> wrote:
> On Wed, Feb 25, 2015 at 08:20:18PM -0800, Evan Gates wrote:
>> Went back and added some comments to expr where I thought it would
>> benefit from extra explanation. Got rid of unnecessary allocations.
>> Used utfnlen() with the match operator to add UTF-8 support. Made some
>> changes for the style guide, then also rearranged a few things that
>> IMO make the code more readable.
>>
>> I added a FIXME? comment. There is a strdup() that is never free()d.
>> Is it worth keeping track of it at a global level just to free when
>> we're done?
>
> BTW, it would have been useful to include the text above in the commit
> message :)
>
> I don't think we should bother with that strdup() at all.
>
From 27c2084dffc2cde6bc44f3d3538ea161e9f4463a Mon Sep 17 00:00:00 2001
From: Evan Gates <evan.ga...@gmail.com>
Date: Thu, 26 Feb 2015 13:44:03 -0800
Subject: [PATCH] keep track of allocations due to enstrdup() in match(), then
free them when we are finished (also spaces->tabs for one indentation
mistake)
---
expr.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/expr.c b/expr.c
index 1d7087b..899b3d6 100644
--- a/expr.c
+++ b/expr.c
@@ -1,6 +1,7 @@
/* See LICENSE file for copyright and license details. */
#include <inttypes.h>
#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
#include "utf.h"
@@ -17,8 +18,24 @@ typedef struct {
intmax_t n;
} Val;
+/* keep track of memory allocated in match() */
+typedef struct Alloc Alloc;
+struct Alloc {
+ Alloc *next;
+ char *s;
+} *allocs;
+
static size_t intlen;
+static char *
+add_alloc(char *s)
+{
+ Alloc *a = emalloc(sizeof(*a));
+ *a = (Alloc){ allocs, s };
+ allocs = a;
+ return s;
+}
+
static void
enan(Val v)
{
@@ -87,13 +104,10 @@ match(Val vstr, Val vregx)
d = strtoimax(s, &p, 10);
if (*s && !*p) /* string matched by subexpression is an integer
*/
return (Val){ NULL, d };
-
- /* FIXME? string is never free()d, worth fixing?
- * need to allocate as it could be in buf1 instead of vstr.s */
- return (Val){ enstrdup(3, s), 0 };
+ return (Val){ add_alloc(enstrdup(3, s)), 0 };
}
regfree(&re);
- str += matches[0].rm_so;
+ str += matches[0].rm_so;
return (Val){ NULL, utfnlen(str, matches[0].rm_eo - matches[0].rm_so) };
}
@@ -250,6 +264,12 @@ parse(char **expr, int exprlen)
else
printf("%"PRIdMAX"\n", valp->n);
+ for (Alloc *p = allocs; p; p = allocs) {
+ allocs = p->next;
+ free(p->s);
+ free(p);
+ }
+
return (valp->s && *valp->s) || valp->n;
}
--
2.3.0
