[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 11/12] libpoke: Add `format`
From: |
Jose E. Marchesi |
Subject: |
Re: [PATCH 11/12] libpoke: Add `format` |
Date: |
Thu, 27 May 2021 22:22:08 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
> 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.
[PATCH 12/12] pvm.jitter: Wrap std string functions, Mohammad-Reza Nabipoor, 2021/05/25
Re: [PATCH 00/12] Add format function, Jose E. Marchesi, 2021/05/27