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

[Francis Giraldeau]

Le 2012-08-14 00:36, David Lutterkort a écrit :
> 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 ?

Ok, let's do that. Then, when the parse starts, rec_state.ast must not
be NULL. The code to create the root AST node is added to rec_process.

> 
>> @@ -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.

Yeah, simpler.

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

The check is also made inside ast_append(), so if ERR_NOMEM is raised,
get_error() will be called twice. Well, it's harmless, but to make it
consistent, it's the right thing to do?

> 
>> @@ -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.

Ho, that's funny, do you see the problem with this code?

done:
    rec_state.ast = ast_root(rec_state.ast);
    if (rec_state.ast != NULL);
        ensure(rec_state.ast->parent == NULL, state->info);
    ... stuff ...
    return rec_state.frames;

error:
    ... stuff ...
    goto done;

Just to avoid the possibility of infinite loop, I added the ensure()
before the done! ;)

Cheers,

Francis

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4489 bytes
Desc: Signature cryptographique S/MIME
URL: <http://listman.redhat.com/archives/augeas-devel/attachments/20120814/fae6d6db/attachment.p7s>