Re: [PATCH] pkl: re-work re-declaration

[Mohammad-Reza Nabipoor]

Hi Jose.

On Mon, Jun 26, 2023 at 07:29:19PM +0200, Jose E. Marchesi wrote:
> 
> > diff --git a/bootstrap.conf b/bootstrap.conf
> > index c5cf0452..7a8e0cec 100644
> > --- a/bootstrap.conf
> > +++ b/bootstrap.conf
> > @@ -106,6 +106,7 @@ libpoke_modules="
> >    tmpdir
> >    vasprintf-posix
> >    vsnprintf-posix
> > +  xvasprintf-posix
> 
> Wouldn't it be worth to try to use vasprintf instead of xvasprintf,
> handling the EOM error in a library (avoiding an abort) friendly way?
> 


Sure; I did it this way because `pkl_env_new' already uses `xzalloc'.
So I will fix `pkl_env_new', too.


> >    xalloc
> >    strstr
> >    lib-symbol-visibility
> > diff --git a/libpoke/pkl-ast.c b/libpoke/pkl-ast.c
> > index 7c2dc7ab..ef170706 100644
> > --- a/libpoke/pkl-ast.c
> > +++ b/libpoke/pkl-ast.c
> > @@ -1500,7 +1500,18 @@ pkl_type_append_to (pkl_ast_node type, int 
> > use_given_name,
> >    if (use_given_name
> >        && PKL_AST_TYPE_NAME (type))
> >      {
> > -      sb_append (buffer, PKL_AST_IDENTIFIER_POINTER (PKL_AST_TYPE_NAME 
> > (type)));
> > +      char *name = PKL_AST_IDENTIFIER_POINTER (PKL_AST_TYPE_NAME (type));
> > +      char *dollar = strchr (name, '$');
> > +
> > +      if (dollar)
> > +        {
> > +          sb_append (buffer, "a previous declaration of ");
> > +          *dollar = '\0';
> > +          sb_append (buffer, name);
> > +          *dollar = '$';
> > +        }
> > +      else
> > +        sb_append (buffer, name);
> >        return;
> >      }
> >  
> 
> Ok so the printed representation of a re-defined type name will always
> be "a previous declaration of WHATEVER".  Does this work well in every
> place where use display type names generated using this function?
>


What do you mean by "work well"? Do you mean "is this user friendly?"
I don't think so.  We can entrich the name with location information
like "WHATEVER (%d-th re-definition of WHATEVER at source.pk:123)".
We replace %d with the generation number.


> > @@ -194,6 +267,7 @@ pkl_env_free (pkl_env env)
> >    if (env)
> >      {
> >        pkl_env_free (env->up);
> > +      env_redecls_free (env, /*rollback_p*/ 1);
> >        free_hash_table (env->hash_table);
> >        free_hash_table (env->units_hash_table);
> >        free (env);


I call `pkl_redecls_free' here in `pkl_env_free'.
`pkl_redecls_free' will go through all the re-declared nodes and rollback
the old definition of them.

That's the reason, I removed the `pkl_env_rollback_renames' invocations
from `libpoke/pkl.c' when there's compile-time error.


> > @@ -230,7 +304,7 @@ pkl_env_register (pkl_env env,
> >  {
> >    pkl_hash *table = get_ns_table (env, namespace);
> >  
> > -  if (register_decl (env->up == NULL, *table, name, decl))
> > +  if (register_decl (env, *table, name, decl))
> 
> Is that first argument supposed to be top_level_p?  Otherwise, this is a
> logic change.  Is it intended?
> 


It is intended.  When do re-define a declaration, I need the `env' to keep
track of re-declarations:

```c
  /* Register DECL in re-defined declarations list.  */
  PKL_AST_DECL_REDECL_CHAIN (decl) = env->redecls;
  env->redecls = decl;
```

> > diff --git a/libpoke/pkl.c b/libpoke/pkl.c
> > index 9da052ed..8367d536 100644
> > --- a/libpoke/pkl.c
> > +++ b/libpoke/pkl.c
> > @@ -348,11 +348,13 @@ pkl_execute_buffer (pkl_compiler compiler,
> >      {
> >        pkl_env_free (compiler->env);
> >        compiler->env = env;
> > +      pkl_env_commit_renames (compiler->env);
> >      }
> > +  else
> > +    pkl_env_rollback_renames (env);
> >    return 1;
> >  
> >   error:
> > -  pkl_env_rollback_renames (compiler->env);
> >    pkl_env_free (env);
> >    return 0;
> 
> Hmm, so the rollback is no longer necessary when some error condition
> leading to this label happen?
> 


`pkl_env_free' does the rollback.
Because compilation failed and we didn't call the `pkl_env_commit_renames',
so we still have the list of `redecls'.

Maybe I have to keep them explicitly.  WDYT?


> > @@ -554,12 +560,14 @@ pkl_execute_file (pkl_compiler compiler, const char 
> > *fname,
> >      {
> >        pkl_env_free (compiler->env);
> >        compiler->env = env;
> > +      pkl_env_commit_renames (compiler->env);
> >      }
> > +  else
> > +    pkl_env_rollback_renames (env);
> >    return 1;
> >  
> >   error:
> >    fclose (fp);
> > -  pkl_env_rollback_renames (compiler->env);
> 
> Likewise?
> 
> >    pkl_env_free (env);


Yes, because of `pkl_env_free'.