[augeas-devel] [PATCH 2/6] Build AST while visiting the parse

[David Lutterkort]

On Sun, 2012-08-12 at 23:37 +0200, Francis Giraldeau wrote:
> The AST records the structure of the parse according to the lens tree. The
> indices of the text matched is recorded for later use.
> ---
>  src/get.c |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)

ACK; one minor nit:

> diff --git a/src/get.c b/src/get.c
> index 734abf6..05663a4 100644
> --- a/src/get.c
> +++ b/src/get.c


> +/* pop the ast from one level if parent is not null */
> +static void ast_pop(struct rec_state *rec_state) {
> +    if (rec_state->ast != NULL && rec_state->ast->parent != NULL)
> +        rec_state->ast = rec_state->ast->parent;

Shouldn't we treat an attempt to pop from an empty tree or from the root
of the tree as an internal error, i.e. add ensure(rec_state->ast != NULL
&& rec_state->ast->parent != NULL, rec_state->state->info) at the top of
ast_pop ?

> @@ -871,6 +979,7 @@ static void visit_terminal(struct lens *lens, size_t start, size_t end,
>          get_terminal(top, lens, state);
>      else
>          parse_terminal(top, lens, state);
> +    ast_append(rec_state, lens, start, end);
>      free_regs(state);
>      state->regs = old_regs;
>      state->nreg = old_nreg;
> @@ -882,6 +991,7 @@ static void visit_enter(struct lens *lens,
>                          void *data) {
>      struct rec_state *rec_state = data;
>      struct state *state = rec_state->state;
> +    struct ast *child;
>  
>      if (state->error != NULL)
>          return;
> @@ -904,6 +1014,9 @@ static void visit_enter(struct lens *lens,
>              ERR_NOMEM(state->span == NULL, state->info);
>          }
>      }
> +    child = ast_append(rec_state, lens, start, end);
> +    if (child != NULL)
> +        rec_state->ast = child;
>   error:
>      return;
>  }

Why not pull the functionality of initializing the tree in rec_state
into ast_append ? In the other place where we are calling it in
visit_terminal, we expect to have rec_state->ast != NULL anyway.

Also, you should do ERR_NOMEM(child == NULL, state->info) after the
ast_append.

> @@ -1151,12 +1265,16 @@ static struct frame *rec_process(enum mode_t mode, struct lens *lens,
>      }
>  
>   done:
> +    if (debugging("cf.get.ast"))
> +        print_ast(rec_state.ast, 0);
>      state->regs = old_regs;
>      state->nreg = old_nreg;
>      jmt_free_parse(visitor.parse);
> +    free_ast(rec_state.ast);

We expect rec_state.ast to be the root of the AST here - do a
ensure(rec_state.ast->parent == NULL) before calling free_ast, and
free_ast(ast_root(rec_state.ast)) to be on the safe side.

David