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

[Jose E. Marchesi]

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

It is mainly a matter of personal preference: I find that thinking in
explicit blocks (as opposed to the implicit ones in c99) helps me to
write better organized code.  Whenever I find myself in a situation like
the one you describe, for example, I have to ask myself whether I'm
structuring the code properly, or I have to factorize, or abstract
something into a function, or...  Sometimes I use explicit blocks just
to introduce new variables, and in my experience these cases are often
later excellent candidates for being abstracted into functions.

Requiring a C99 compiler is nowadays quite reasonable, anyway, and I
won't oppose using the newer style in new code.  But preferably keeping
some level of consistency with the surrounding code...
    
    >     +          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)

Yes, given the change in the pkl_error message it makes sense to have
this as part of the same 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");
    
    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 ?

I think we can provide an useful message in every case.  Let's see... we
have:

1. When (p[2] >= '0' && p[2] <= '9') holds false.  In that case, we can
   say something like "expected number of bits".

2. When the format letter is not 's', 'i' nor 'u'.  In that case, we can
   say something like "invalid format specifier '%c'".

    > ...
    > 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)
    
Thank you, and have a good trip back! :)