Re: [PATCH 11/12] libpoke: Add `format`

[Jose E. Marchesi]

> On Thu, May 27, 2021 at 08:29:40PM +0200, Jose E. Marchesi wrote:
>> 
>> Hi Mohammad.
>> 
>> Before reviewing this patch in detail I have got a general question for
>> you.
>> 
>> Have you considered to use a single AST node type for both PRINT and
>> FORMAT?  In that case, what led you to not use that approach?
>> 
>> The same applies to the corresponding anal, trans, etc handlers.
>> 
>
> My initial plan was
>   - copy the PRINT AST node and rename it to FORMAT
>   - Implement `format`
>   - Re-write `print` using `format`: `print (format (...))`
>   - Simplify the PRINT AST node to just a single pvm_val of type string
>   - Re-write codegen for PRINT to be able to print "fat strings"
>
> But after implementing the formater for array, I realized that printing
> a large array is much more efficient than formating a large array and
> then printing that big string.

I like that plan.

> For sure the current patch introduces code duplication for all phases
> except the codegen phase (it is just literal copy of the printing code
> with s/print/format).
>
> I decided to postpone the unification of the two nodes, because maybe
> there's a chance of getting ride of `print` by using lazy-strings and
> `format`.

Well, precisely: if we unify the AST nodes now, into something that can
accomodate both PRINT and FORMAT, then it will be just a matter of
renaming it when it is time for PRINT to go away :)

There is a detail however: print statements are... statements, and
therefore PKL_AST_PRINT_STMT lays between PKL_AST_FIRST_STMT and
PKL_AST_LAST_STMT.

On the other side, `format' is an expression that returns a value, not a
statement.

To resolve this, we could have two AST node types, still sharing the
implementation:

   PKL_AST_FORMAT
   - Contains all the attributes like fmt, types, etc.

   PKL_AST_PRINT_STMT
    - Contains a PKL_AST_FORMAT.

That way, we will still need phase handlers for PKL_AST_FORMAT.  But the
real work can then be in functions in pkl-ast.c, used by both
PKL_AST_FORMAT and PKL_AST_PRINT_STMT.  Or macros.

WDYT?

>
> The only part that requires reviewing is the codegen part.
>
> Should I unify the AST node in this patch, or I can do it in a subsequent
> patch?

I would really like to avoid all that code replication, even if it is
temporary.