On Sun, 2010-08-22 at 10:29 -0400, Francis Giraldeau wrote:
> Hi,
>
> I did modified the lens square patch:
>
> * renamed *tip to *square for clarity
> * code cleanup and formating
> * split from other changes
> * New changelog to explain changes
> * Squash patches into one
>
> The code is in branch square-release :
>
> http://github.com/giraldeau/augeas/tree/square-release
This looks very good, almost ready to merge. There were a few problems I
found, and I attached a patch against your branch that corrects all but
one of them. In more detail:
(1) lns_make_square vs. memory management
Memory mgmt in Augeas is a bit of a mess, and I'd love to switch to a gc
for a lot of the backend memory mgmt; but that it's unlikely I'll get
around to it any time soon.
For reference counted structs, you should never explicitly call the
free_xxx routine, since that will complain if the ref count on the
struct is not zero yet. Instead, use unref; e.g., instead of calling
free_value(v) you should do unref(v, value)
The other issue is that you need to pay close attention where the
reference for newly created objects go. Most make routines consume the
reference, i.e. they incorporate the object without increasing its
refcount. The right pattern to use is
lens = lns_make_prim(L_KEY, ref(info), ref(reg));
if (some_error(lens))
goto error;
done:
unref(info, info);
unref(reg, regexp);
error:
unref(lens, lens);
goto done;
A lot of the existing code gets that wrong, too. The idea is that
regardless of whether the function succeeds or errors out, we consume
exactly one reference for info and reg.
(2) Typechecking was turned off
Combinators like lns_make_concat take a flag to indicate whether they
should do typechecking or not. That flag needs to be passed through from
lns_square in builtin.c
(3) Some allocation error
When you run the test through valgrind, e.g, with
libtool --mode=execute valgrind ./src/augparse --nostdinc
tests/modules/pass_square.aug
it reports a bad memory access deep in the bowels of put - I haven't
been able to figure out where that comes from.
But with the attached patch, and a solution to (3), this will be ready
to merge.
David
>From 1b8eda735b6b8bfecf08e8b817e09f4288bccdc3 Mon Sep 17 00:00:00 2001
From: David Lutterkort <[email protected]>
Date: Fri, 27 Aug 2010 14:13:41 -0700
Subject: [PATCH] Fix a few problems with the square lens
---
src/builtin.c | 7 +++-
src/lens.c | 78 ++++++++++++++++++++++-------------------
src/lens.h | 3 +-
tests/modules/pass_square.aug | 28 +++++++-------
4 files changed, 63 insertions(+), 53 deletions(-)
diff --git a/src/builtin.c b/src/builtin.c
index e14955d..1b339e3 100644
--- a/src/builtin.c
+++ b/src/builtin.c
@@ -87,10 +87,13 @@ static struct value *lns_counter(struct info *info, struct value *str) {
}
/* V_REGEXP -> V_LENS -> V_LENS */
-static struct value *lns_square(struct info *info, struct value *rxp, struct value *lns) {
+static struct value *lns_square(struct info *info, struct value *rxp,
+ struct value *lns) {
assert(rxp->tag == V_REGEXP);
assert(lns->tag == V_LENS);
- return lns_make_square(ref(info), ref(rxp->regexp), ref(lns->lens));
+ int check = info->error->aug->flags & AUG_TYPE_CHECK;
+
+ return lns_make_square(ref(info), ref(rxp->regexp), ref(lns->lens), check);
}
static struct value *make_exn_lns_error(struct info *info,
diff --git a/src/lens.c b/src/lens.c
index 3270c11..a90defb 100644
--- a/src/lens.c
+++ b/src/lens.c
@@ -395,56 +395,62 @@ struct value *lns_make_maybe(struct info *info, struct lens *l, int check) {
return make_lens_value(lens);
}
+/* Build a square lens as
+ * key REG . lns . del REG MATCHED
+ * where MATCHED is whatever the key lens matched (the inability to express
+ * this with other lenses makes the square primitve necessary
+ */
struct value *lns_make_square(struct info *info,
struct regexp *reg,
- struct lens *lns) {
- struct value *key = NULL, *del = NULL, *cnt1 = NULL, *cnt2 = NULL, *res = NULL;
+ struct lens *lns, int check) {
+ struct value *key = NULL, *del = NULL;
+ struct value *cnt1 = NULL, *cnt2 = NULL, *res = NULL;
struct lens *sqr = NULL;
- key = lns_make_prim(L_KEY, info, reg, NULL);
- if (EXN(key)) {
- return key;
- }
-
- del = lns_make_prim(L_DEL, info, reg, NULL);
- if (EXN(del)) {
- free_value(key);
- return del;
- }
+ res = lns_make_prim(L_KEY, ref(info), ref(reg), NULL);
+ if (EXN(res))
+ goto error;
+ key = res;
- // Need to ref once more to avoid double free...
- ref(reg);
+ res = lns_make_prim(L_DEL, ref(info), ref(reg), NULL);
+ if (EXN(res))
+ goto error;
+ del = res;
// typechecking is handled when concatenating lenses
- cnt1 = lns_make_concat(info, key->lens, lns, 0);
- if (EXN(cnt1)) {
- free_value(key);
- free_value(del);
- return cnt1;
- }
- cnt2 = lns_make_concat(info, cnt1->lens, del->lens, 0);
- if (EXN(cnt2)) {
- free_value(key);
- free_value(del);
- free_value(cnt1);
- return cnt2;
- }
-
- sqr = make_lens_unop(L_SQUARE, info, cnt2->lens);
- sqr->atype = ref(cnt2->lens->atype);
- sqr->ctype = ref(cnt2->lens->ctype);
- sqr->ktype = ref(cnt2->lens->ktype);
- sqr->vtype = ref(cnt2->lens->vtype);
+ res = lns_make_concat(ref(info), ref(key->lens), ref(lns), check);
+ if (EXN(res))
+ goto error;
+ cnt1 = res;
+
+ res = lns_make_concat(ref(info), ref(cnt1->lens), ref(del->lens), check);
+ if (EXN(res))
+ goto error;
+ cnt2 = res;
+
+ sqr = make_lens_unop(L_SQUARE, ref(info), ref(cnt2->lens));
+ ERR_NOMEM(sqr == NULL, info);
+
+ for (int t=0; t < ntypes; t++)
+ ltype(sqr, t) = ref(ltype(cnt2->lens, t));
sqr->recursive = cnt2->lens->recursive;
sqr->rec_internal = cnt2->lens->rec_internal;
sqr->consumes_value = cnt2->lens->consumes_value;
res = make_lens_value(sqr);
+ ERR_NOMEM(res == NULL, info);
+ sqr = NULL;
- FREE(key);
- FREE(del);
- FREE(cnt1);
- FREE(cnt2);
+ error:
+ unref(info, info);
+ unref(reg, regexp);
+ unref(lns, lens);
+
+ unref(key, value);
+ unref(del, value);
+ unref(cnt1, value);
+ unref(cnt2, value);
+ unref(sqr, lens);
return res;
}
diff --git a/src/lens.h b/src/lens.h
index 8b14eee..8a8c1ce 100644
--- a/src/lens.h
+++ b/src/lens.h
@@ -144,7 +144,8 @@ struct value *lns_make_plus(struct info *, struct lens *,
int check);
struct value *lns_make_maybe(struct info *, struct lens *,
int check);
-struct value *lns_make_square(struct info *, struct regexp *, struct lens *);
+struct value *lns_make_square(struct info *, struct regexp *, struct lens *,
+ int check);
/* Pretty-print a lens */
char *format_lens(struct lens *l);
diff --git a/tests/modules/pass_square.aug b/tests/modules/pass_square.aug
index 7ee9c9c..f7a37f9 100644
--- a/tests/modules/pass_square.aug
+++ b/tests/modules/pass_square.aug
@@ -1,7 +1,7 @@
-module Pass_square =
+module Pass_square =
(* Utilities lens *)
-let dels (s:string) = del s s
+let dels (s:string) = del s s
(************************************************************************
* Regular square lens
@@ -14,7 +14,7 @@ test sqr0 get "xyxxyxxyx" = { "x" = "y" }{ "x" = "y" }{ "x" = "y" }
test sqr0 put "xyx" after set "/x[3]" "z" = "xyxxzx"
(* test mismatch tag *)
-test sqr0 get "xya" = *
+test sqr0 get "xya" = *
(* Test regular expression matching with multiple groups *)
let body = del /([f]+)([f]+)/ "ff" . del /([g]+)([g]+)/ "gg"
@@ -22,7 +22,7 @@ let sqr1 = [ square /([a-b]*)([a-b]*)([a-b]*)/ body . del /([x]+)([x]+)/ "xx" ]
test sqr1 get "aaffggaaxxbbffggbbxx" = { "aa" }{ "bb" }
test sqr1 get "affggaxx" = { "a" }
-test sqr1 put "affggaxx" after clear "/b" = "affggaxxbffggbxx"
+test sqr1 put "affggaxx" after clear "/b" = "affggaxxbffggbxx"
(* Test XML like elements up to depth 2 *)
let b = del ">" ">" . del /[a-z ]*/ "" . del "</" "</"
@@ -36,8 +36,8 @@ test xml get "<a></a><b></b>" = { "a" }{ "b" }
(* test error on mismatch tag *)
test xml get "<a></a><b></c>" = *
-(* test get nested tags of depth 2 *)
-test xml2 get "<a><b></b><c></c></a>" =
+(* test get nested tags of depth 2 *)
+test xml2 get "<a><b></b><c></c></a>" =
{ "a"
{ "b" }
{ "c" }
@@ -55,12 +55,12 @@ test xml2 put "<a></a>" after clear "/x/y/z" = *
(* Basic element *)
let xml_element (body:lens) =
- let g = del ">" ">" . body . del "</" "</" in
+ let g = del ">" ">" . body . del "</" "</" in
[ del "<" "<" . square /[a-z]+/ g . del ">" ">" ] *
-
+
let rec xml_rec = xml_element xml_rec
-test xml_rec get "<a><b><c><d><e></e></d></c></b></a>" =
+test xml_rec get "<a><b><c><d><e></e></d></c></b></a>" =
{ "a"
{ "b"
{ "c"
@@ -81,23 +81,23 @@ test xml_rec get "<a><b></b><c></c><d></d><e></e></a>" =
test xml_rec put "<a></a><b><c></c></b>" after clear "/x/y/z" = "<a></a><b><c></c></b><x><y><z></z></y></x>"
-(* mismatch tag *)
+(* mismatch tag *)
test xml_rec get "<a></c>" = *
test xml_rec get "<a><b></b></c>" = *
-test xml_rec get "<a><b></c></a>" = *
+test xml_rec get "<a><b></c></a>" = *
(* test ctype_nullable and typecheck *)
let rec z = [ square "ab" z? ]
test z get "abab" = { "ab" }
(* test tip handling when using store inside body *)
-let c (body:lens) =
+let c (body:lens) =
let sto = store "c" . body* in
[ square "ab" sto ]
let rec cc = c cc
-
-test cc get "abcabcabab" =
+
+test cc get "abcabcabab" =
{ "ab" = "c"
{ "ab" = "c" }
}
--
1.7.2.2
_______________________________________________
augeas-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/augeas-devel
