[augeas-devel] [PATCH] Generic square lens v2
David Lutterkort
lutter at redhat.com
Fri Aug 10 22:25:43 UTC 2012
On Thu, 2012-08-09 at 14:18 +0200, Francis Giraldeau wrote:
> Lens square now takes lenses instead of regular expressions as arguments, as follow:
>
> square left body right
>
> The left and right lenses must accept the same language. The left lens can be
> either key or del, while the right lens must be del.
This is getting very close, but there's still a few issues with the
patch. For one, there's a ton of whitespace changes in the patch - if
you really want them, please submit them as a separate patch that _only_
changes ws. It would be much easier to review this if it were (at least)
two patches: one that introduces and builds an AST, and a second one
that makes the changes to the square lens.
When I run valgrind on the two pass_square_* tests, there's a few
allocation problems (valgrind output is attached)
Some more comments inline:
> diff --git a/src/get.c b/src/get.c
> index c1e206c..508c4f4 100644
> --- a/src/get.c
> +++ b/src/get.c
>
> @@ -109,6 +119,74 @@ struct rec_state {
> #define REG_MATCHED(state) (REG_VALID(state) \
> && (state)->regs->start[(state)->nreg] >= 0)
>
> +/*
> + * AST utils
> + */
> +static struct ast *make_ast(struct lens *lens) {
> + struct ast *ast = NULL;
> +
> + CALLOC(ast, 1);
I am trying to get off using CALLOC and use ALLOC and ALLOC_N instead,
since they are much safer.
> + if (ast == NULL)
> + return NULL;
> + ast->lens = lens;
> + ast->capacity = 4;
> + CALLOC(ast->children, ast->capacity);
Same here
> @@ -723,28 +800,81 @@ static struct skel *parse_subtree(struct lens *lens, struct state *state,
> return make_skel(lens);
> }
>
> +static int square_match(struct lens *lens, char *left, char *right) {
> + int cmp = 0;
> + struct lens *concat = NULL;
> +
> + concat = lens->child;
> + if (left == NULL || right == NULL)
> + return cmp;
Why do we treat NULL == "anything" ?
> +#define combine_assign(src, dst, rec_state) \
I'd find combine_assigne(dst, src, rec_state) more intuitive (since
you'd write dst = src)
> @@ -1080,7 +1230,14 @@ static void visit_exit(struct lens *lens,
> } else {
> top_frame(rec_state)->lens = lens;
> }
> + /* pop the ast from one level if parent is not null */
> + if (rec_state->ast != NULL && rec_state->ast->parent != NULL)
> + rec_state->ast = rec_state->ast->parent;
> error:
> + if (rsqr != NULL)
> + FREE(rsqr);
> + if (lsqr != NULL)
> + FREE(lsqr);
You don't need these checks - it's safe to pass NULL into free (and FREE)
> diff --git a/src/info.h b/src/info.h
> index 1f38cdc..a08c15c 100644
> --- a/src/info.h
> +++ b/src/info.h
> @@ -26,6 +26,7 @@
> #include <stdio.h>
> #include <stdint.h>
> #include <stdlib.h>
> +#include "errcode.h"
Why include that in info.h ? It's only needed in info.c, and should be
included there.
> diff --git a/src/lens.c b/src/lens.c
> index 0134996..c43d4ad 100644
> --- a/src/lens.c
> +++ b/src/lens.c
> @@ -39,53 +39,56 @@ static const int const type_offs[] = {
> offsetof(struct lens, ktype),
> offsetof(struct lens, vtype)
> };
> -static const int ntypes = sizeof(type_offs)/sizeof(type_offs[0]);
> +static const int ntypes = sizeof(type_offs) / sizeof(type_offs[0]);
>
> -static const char *lens_type_names[] =
> - { "ctype", "atype", "ktype", "vtype" };
> +static const char *lens_type_names[] = { "ctype", "atype", "ktype", "vtype" };
Gratuitous whitespace changes; should go into a separate patch if they
are really wanted.
> #define ltype(lns, t) *((struct regexp **) ((char *) lns + type_offs[t]))
>
> -static struct value * typecheck_union(struct info *,
> - struct lens *l1, struct lens *l2);
> -static struct value *typecheck_concat(struct info *,
> - struct lens *l1, struct lens *l2);
> -static struct value *typecheck_iter(struct info *info, struct lens *l);
> -static struct value *typecheck_maybe(struct info *info, struct lens *l);
> +static struct value *
> +typecheck_union(struct info *, struct lens *l1, struct lens *l2);
> +static struct value *
> +typecheck_concat(struct info *, struct lens *l1, struct lens *l2);
> +static struct value *
> +typecheck_square(struct info *, struct lens *l1, struct lens *l2);
> +static struct value *
> +typecheck_iter(struct info *info, struct lens *l);
> +static struct value *
> +typecheck_maybe(struct info *info, struct lens *l);
More whitespace changes ? Here it actually starts getting annoying to
check that it's really just ws that changed.
> /* Lens names for pretty printing */
> /* keep order in sync with enum type */
> -static const char *const tags[] = {
> - "del", "store", "value", "key", "label", "seq", "counter",
> - "concat", "union",
> - "subtree", "star", "maybe", "rec", "square"
> -};
> +static const char * const tags[] = { "del", "store", "value", "key", "label",
> + "seq", "counter", "concat", "union", "subtree", "star", "maybe", "rec",
> + "square" };
Again, only whitespace ?
> #define ltag(lens) (tags[lens->tag - L_DEL])
>
> -static const struct string digits_string = {
> - .ref = REF_MAX, .str = (char *) "[0123456789]+"
> -};
> -static const struct string *const digits_pat = &digits_string;
> +static const struct string digits_string = { .ref = REF_MAX, .str =
> + (char *) "[0123456789]+" };
> +static const struct string * const digits_pat = &digits_string;
>
> -char *format_lens(struct lens *l) {
> +char *
> +format_lens(struct lens *l) {
> char *inf = format_info(l->info);
> char *result;
>
> xasprintf(&result, "%s[%s]%s", tags[l->tag - L_DEL], inf,
> - l->recursive ? "R" : "r");
> + l->recursive ? "R" : "r");
> free(inf);
> return result;
> }
>
> #define BUG_LENS_TAG(lns) bug_lens_tag(lns, __FILE__, __LINE__)
>
> -static void bug_lens_tag(struct lens *lens, const char *file, int lineno) {
> +static void
> +bug_lens_tag(struct lens *lens, const char *file, int lineno) {
> char *s = format_lens(lens);
>
> if (lens != NULL && lens->info != NULL && lens->info->error != NULL) {
> bug_on(lens->info->error, file, lineno, "Unexpected lens tag %s", s);
> - } else {
> + }
> + else {
Please don't do that; I really want '} else {' on one line in these constructs.
> /* We are really screwed */
> assert(0);
> }
> @@ -98,8 +101,8 @@ static void bug_lens_tag(struct lens *lens, const char *file, int lineno) {
> * Return NULL if REGEXP is valid, if the regexp REGEXP has syntax errors,
> * return an exception.
> */
> -static struct value *str_to_fa(struct info *info, const char *pattern,
> - struct fa **fa, int nocase) {
> +static struct value *
> +str_to_fa(struct info *info, const char *pattern, struct fa **fa, int nocase) {
> int error;
> struct value *exn = NULL;
> size_t re_err_len;
> @@ -127,22 +130,22 @@ static struct value *str_to_fa(struct info *info, const char *pattern,
> regerror(error, NULL, re_err, re_err_len);
> exn_printf_line(exn, "%s", re_err);
>
> - done:
> - free(re_str);
> + done: free(re_str);
> free(re_err);
> return exn;
> - error:
> - fa_free(*fa);
> + error: fa_free(*fa);
> *fa = NULL;
> exn = info->error->exn;
> goto done;
> }
>
> -static struct value *regexp_to_fa(struct regexp *regexp, struct fa **fa) {
> +static struct value *
> +regexp_to_fa(struct regexp *regexp, struct fa **fa) {
> return str_to_fa(regexp->info, regexp->pattern->str, fa, regexp->nocase);
> }
>
> -static struct lens *make_lens(enum lens_tag tag, struct info *info) {
> +static struct lens *
> +make_lens(enum lens_tag tag, struct info *info) {
> struct lens *lens;
> make_ref(lens);
> lens->tag = tag;
> @@ -151,8 +154,8 @@ static struct lens *make_lens(enum lens_tag tag, struct info *info) {
> return lens;
> }
>
> -static struct lens *make_lens_unop(enum lens_tag tag, struct info *info,
> - struct lens *child) {
> +static struct lens *
> +make_lens_unop(enum lens_tag tag, struct info *info, struct lens *child) {
> struct lens *lens = make_lens(tag, info);
> lens->child = child;
> lens->value = child->value;
> @@ -160,11 +163,12 @@ static struct lens *make_lens_unop(enum lens_tag tag, struct info *info,
> return lens;
> }
>
> -typedef struct regexp *regexp_combinator(struct info *, int, struct regexp **);
> +typedef struct regexp *
> +regexp_combinator(struct info *, int, struct regexp **);
>
> -static struct lens *make_lens_binop(enum lens_tag tag, struct info *info,
> - struct lens *l1, struct lens *l2,
> - regexp_combinator *combinator) {
> +static struct lens *
> +make_lens_binop(enum lens_tag tag, struct info *info, struct lens *l1,
> + struct lens *l2, regexp_combinator *combinator) {
> struct lens *lens = make_lens(tag, info);
> int n1 = (l1->tag == tag) ? l1->nchildren : 1;
> struct regexp **types = NULL;
All these changes are whitespace changes. NAK to that in a patch that
also changes functionality.
Ok, I am stopping to look at this now; please remove the ws changes from
the patch and regenerate it.
David
More information about the augeas-devel
mailing list