[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: PATCH: provide %c and %u8c formatting in pickle printf()
From: |
Jose E. Marchesi |
Subject: |
Re: PATCH: provide %c and %u8c formatting in pickle printf() |
Date: |
Mon, 30 Sep 2019 19:22:36 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Hello Henner.
Thank you for the patch! Please find my comments below.
[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.)]
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?
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.
/* 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.
+ 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.
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");
...
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 :)