Re: PATCH: provide %c and %u8c formatting in pickle printf()

[Jose E. Marchesi]

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 :)