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

[Jose E. Marchesi]

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

Thanks.

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

Hmm, I think the current wording is good enough.  I don't think it is
very useful to track locations of previous definitions.  No other
interactive languages do that AFAIK.

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

Ok.

>> > @@ -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;
> ```

Ok.  So the meaning of the first argument is different.

>> > 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?

Maybe a comment before pkl_env_free?

In general, I would like to have more comments in that file, explaining
how the renaming schema works...

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