Hi Jose,
> Thank you for the patch! Please find my comments below.
Thanks for looking at the patch.
Comments below. I'll review the updated HACKING and reformat the patches as several sets.
>
> [For future submissions, please inline your patches in the mail body, or
> attach them with mime type text/x-patch or text/x-diff. This eases
> review. Thanks! (I added a note about this to HACKING.)]
Ok (I was mostly cautious because there was a time when mail-clients/servers liked to mess with mime-content that remotely looked like text)
>
> diff --git a/src/
pk-dump.pk b/src/
pk-dump.pk> index 722b01f..21c0947 100644
> --- a/src/
pk-dump.pk> +++ b/src/
pk-dump.pk> @@ -76,6 +76,20 @@ defun dump = (off64 from = pk_dump_offset,
> printf ("%u8x", int<8> @ (offset + o));
> o = o + 1#B;
> }
> + if (ascii)
> + {
> + print(" ");
> + o = 0#B;
> + while (o < step && offset + o < top)
> + {
> + defvar v = int<8> @ (offset + o);
> + if (v < ' ')
> + print(".");
> + else
> + printf ("%c", v);
> + o = o + 1#B;
> + }
> + }
> print "\n";
>
> offset = offset + step;
>
> Nice. You must know that by this code you become, as far as I know, the
> second Poke programmer ever :)
:)
>
> However, what about putting this change in a separated commit?
Sounds good.
>
> diff --git a/src/pkl-trans.c b/src/pkl-trans.c
> index 5ef95de..2feda23 100644
> --- a/src/pkl-trans.c
> +++ b/src/pkl-trans.c
> @@ -677,6 +677,7 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_print_stmt)
> PKL_AST_PRINT_STMT_PREFIX (print_stmt) = prefix;
> }
>
> + const char *msg = "";
>
> Please place declarations at the beginning of blocks.
I noticed that you use do this in your code, and I am fine to adapt.
And in this case, it is not a big deal.
However, I typically like to have things 'const' as much as possible, so that means that variable declarations and assignments of integer and other non-pointers I like to have declared where used which often can't be at the beginning of a block due to the value only calculatable later. Is there a strong reason to prefer C89-style ?
(I have a octal and hex escape patch pending, in which I like to use this kind of style as well. I could re-formulate it with non-const values, but I think, having variables only visible once they are used and having 'const' adds readability).
>
> /* Process the format string. */
> for (types = NULL, ntag = 0, arg = args;
> *p != '\0' && args;
> @@ -702,6 +703,13 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_print_stmt)
> PKL_AST_LOC (atype) = PKL_AST_LOC (print_fmt);
> types = pkl_ast_chainon (types, atype);
> break;
> + case 'c':
> + p += 2;
> + PKL_AST_PRINT_STMT_ARG_BASE (arg) = 256;
>
> If 256 is arbitrary, please say so explicitly with /* Arbitrary. */ as
> it is done in the %s handler.
Noted, will update my patches.
>
> + atype = pkl_ast_make_integral_type (PKL_PASS_AST, 8, 0);
> + PKL_AST_LOC (atype) = PKL_AST_LOC (print_fmt);
> + types = pkl_ast_chainon (types, atype);
> + break;
> case 'i':
> case 'u':
> {
> @@ -723,7 +731,10 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_print_stmt)
> }
>
> if (bits > 64)
> + {
> + msg = " (more than 64 bits)";
> goto invalid_tag;
> + }
>
>
> I think it would be better to put this change in a separated commit.
Sounds good, though if we go with always trailing error message (see comment below), it should probably be included here.
(though two consecutive commits are also fine)
> Also, please use gettextized strings to allow internationalization,
> i.e. use:
>
> msg = _(" (more than 64 bits)");
>
> I know I know, I'm not that consistent with that myself... but I try X)
>
> switch (p[base_idx])
> {
> @@ -731,7 +742,16 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_print_stmt)
> case 'o': PKL_AST_PRINT_STMT_ARG_BASE (arg) = 8; break;
> case 'd': PKL_AST_PRINT_STMT_ARG_BASE (arg) = 10; break;
> case 'x': PKL_AST_PRINT_STMT_ARG_BASE (arg) = 16; break;
> + case 'c':
> + PKL_AST_PRINT_STMT_ARG_BASE (arg) = 256;
> + if (bits != 8)
> + {
> + msg = " (char format only makes sense with 8 bits)";
>
> Ditto.
>
> + goto invalid_tag;
> + }
> + break;
> default:
> + msg = " (invalid base)";
> goto invalid_tag;
> }
>
> @@ -746,7 +766,10 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_print_stmt)
> p += 4;
> }
> else
> + {
> + msg = " (bits expected)";
> goto invalid_tag;
> + }
> break;
> }
> default:
> @@ -787,7 +810,7 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_print_stmt)
>
> invalid_tag:
> pkl_error (PKL_PASS_AST, PKL_AST_LOC (print_fmt),
> - "invalid %%- tag in format string");
> + "invalid %%- tag in format string%s", msg);
>
> I would much prefer to not have the leading space in `msg' and to insert
> it here, i.e. something like:
>
> msg = _("bits expected");
Yes. The reason why I did it this way is because there is not always a msg set, only some cases
have a meaningful message. Should we have some default-message so that we don't have a
random colon with a trailing space in the message ?
> ...
> pkl_error (..., _("invalid %%- tag in format string: %s"), msg);
>
> WDYT?
>
> PKL_TRANS_PAYLOAD->errors++;
> PKL_PASS_ERROR;
> }
> diff --git a/src/pvm.jitter b/src/pvm.jitter
> index 3ab6df7..7251927 100644
> --- a/src/pvm.jitter
> +++ b/src/pvm.jitter
> @@ -312,7 +312,12 @@ late-header-c
> { \
> int prec = 0; \
> \
> - if (JITTER_ARGN1 == 16) \
> + if (JITTER_ARGN1 == 256) \
> + { \
> + fmt[4] = 'c'; \
> + prec = 1; \
> + } \
> + else if (JITTER_ARGN1 == 16) \
> { \
> fmt[4] = 'x'; \
> prec = (JITTER_ARGN0 / 4) + ((JITTER_ARGN0 % 4) != 0); \
>
> Other than the above, it looks great :)
Thanks. I'll update the patchset (currently travelling back from Bordeaux, so might take a day)
Cheers,
Henner.