Re: [PATCH] libpoke: Enable octal and hex \-sequence in string literals

[Jose E. Marchesi]

Hi Mohammad.

Thanks for the patch.

> 2020-10-31  Mohammad-Reza Nabipoor  <m.nabipoor@yahoo.com>
>
>       * libpoke/pkl-trans.c (pkl_trans1_ps_string): Add support for octal
>       and hexadecimal escape sequences.
>       * testsuite/poke.pkl/strings-esc-1.pk: New test.
>       * testsuite/poke.pkl/strings-esc-2.pk: Likewise.
>       * testsuite/poke.pkl/strings-esc-diag-1.pk: Likewise.
>       * testsuite/poke.pkl/strings-esc-diag-2.pk: Likewise.
>       * testsuite/poke.pkl/strings-esc-diag-3.pk: Likewise.
>       * testsuite/poke.pkl/string-diag-1.pk: `\x` is now a valid sequence;
>       use `\z` as an invalid escape sequence.
>       * testsuite/Makefile.am (EXTRA_DIST): Add new tests.
> ---
>
> Hi, Jose!
>
> I have two questions:
>   - Should I break test cases further?

I don't think so.  They seem to be right to me.

But I would expect to see a couple of texts intermixing escape sequences
with normal text in the format string.

>   - `make syntax-check` fails for `isodigit` and `xdigit` macro definitions
>     in `libpoke/pkl-trans.c` (`rockdabootism_missing_space`).
>     What can I do to get rid of that?

Easy: use upper-case letters in macro names, which is the de-facto
standard we use anyway (but not documented in HACKING, I'm taking care
of that.)

>
>
> Thanks,
> Mohammad-Reza
>
>  
>  ChangeLog                                |  13 +++
>  libpoke/pkl-trans.c                      | 120 ++++++++++++++++++-----
>  testsuite/Makefile.am                    |   5 +
>  testsuite/poke.pkl/string-diag-1.pk      |   2 +-
>  testsuite/poke.pkl/strings-esc-1.pk      |  22 +++++
>  testsuite/poke.pkl/strings-esc-2.pk      |  22 +++++
>  testsuite/poke.pkl/strings-esc-diag-1.pk |   3 +
>  testsuite/poke.pkl/strings-esc-diag-2.pk |   3 +
>  testsuite/poke.pkl/strings-esc-diag-3.pk |   5 +
>  9 files changed, 169 insertions(+), 26 deletions(-)
>  create mode 100644 testsuite/poke.pkl/strings-esc-1.pk
>  create mode 100644 testsuite/poke.pkl/strings-esc-2.pk
>  create mode 100644 testsuite/poke.pkl/strings-esc-diag-1.pk
>  create mode 100644 testsuite/poke.pkl/strings-esc-diag-2.pk
>  create mode 100644 testsuite/poke.pkl/strings-esc-diag-3.pk
>
> diff --git a/ChangeLog b/ChangeLog
> index ec713c33..26525210 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,16 @@
> +2020-10-31  Mohammad-Reza Nabipoor  <m.nabipoor@yahoo.com>
> +
> +     * libpoke/pkl-trans.c (pkl_trans1_ps_string): Add support for octal
> +     and hexadecimal escape sequences.
> +     * testsuite/poke.pkl/strings-esc-1.pk: New test.
> +     * testsuite/poke.pkl/strings-esc-2.pk: Likewise.
> +     * testsuite/poke.pkl/strings-esc-diag-1.pk: Likewise.
> +     * testsuite/poke.pkl/strings-esc-diag-2.pk: Likewise.
> +     * testsuite/poke.pkl/strings-esc-diag-3.pk: Likewise.
> +     * testsuite/poke.pkl/string-diag-1.pk: `\x` is now a valid sequence;
> +     use `\z` as an invalid escape sequence.
> +     * testsuite/Makefile.am (EXTRA_DIST): Add new tests.
> +
>  2020-10-28  Mohammad-Reza Nabipoor  <m.nabipoor@yahoo.com>
>  
>       * bootstrap.conf: Add `lib-symbol-visibility` module to `libpoke`.
> diff --git a/libpoke/pkl-trans.c b/libpoke/pkl-trans.c
> index 18366ca0..ce744568 100644
> --- a/libpoke/pkl-trans.c
> +++ b/libpoke/pkl-trans.c
> @@ -20,6 +20,7 @@
>  
>  #include <gettext.h>
>  #define _(str) gettext (str)
> +#include <ctype.h>
>  #include <stdio.h>
>  #include <string.h>
>  #include <xalloc.h>
> @@ -304,6 +305,10 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_string)
>    size_t string_length, i;
>    bool found_backslash = false;
>  
> +#define isodigit(c) ((unsigned)(c) - '0' < 8) // is octal digit

Please do not use C++ comments.

> +#define xdigit(x)  \
> +  ((unsigned)(x) - '0' < 10 ? (x) - '0' : ((x) | 0x20) - 'a' + 10)
> +
>    /* Please keep this code in sync with the string printer in
>       pvm-val.c:pvm_print_val.  */
>  
> @@ -311,27 +316,46 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_string)
>       \-expansion, and report errors in the contents of the string.  */
>    for (p = string_pointer, string_length = 0; *p != '\0'; ++p)
>      {
> -      if (p[0] == '\\')
> +      string_length++;
> +      if (*p != '\\')
> +        continue;
> +
> +      found_backslash = true;
> +      ++p;
> +
> +      if (isodigit (p[0]))
> +        {
> +          if (isodigit (p[1]))
> +            p += isodigit (p[2]) && p[0] <= '3' ? 2 // reason: 0377 == 0xff

Likewise.

> +                                                : 1;
> +          continue;
> +        }
> +
> +      switch (*p)
>          {
> -          switch (p[1])
> +        case '\\':
> +        case 'n':
> +        case 't':
> +        case '"':
> +          break;
> +        case 'x':
> +          ++p;
> +          if (!isxdigit (p[0]))
>              {
> -            case '\\':
> -            case 'n':
> -            case 't':
> -            case '"':
> -              string_length++;
> -              break;
> -            default:
>                PKL_ERROR (PKL_AST_LOC (string),
> -                         "invalid \\%c sequence in string", p[1]);
> +                         _ ("\\x used with no following hex digits"));
>                PKL_TRANS_PAYLOAD->errors++;
>                PKL_PASS_ERROR;
>              }
> -          p++;
> -          found_backslash = true;
> +          if (isxdigit (p[1]))
> +            ++p;
> +          break;
> +        default:
> +          PKL_ERROR (PKL_AST_LOC (string),
> +                     _ ("invalid \\%c sequence in string"), *p);
> +          PKL_TRANS_PAYLOAD->errors++;
> +          PKL_PASS_ERROR;
>          }
> -      else
> -        string_length++;
>      }
>  
>    if (!found_backslash)
> @@ -342,24 +366,70 @@ PKL_PHASE_BEGIN_HANDLER (pkl_trans1_ps_string)
>  
>    for (p = string_pointer, i = 0; *p != '\0'; ++p, ++i)
>      {
> -      if (p[0] == '\\')
> +      if (*p != '\\')
>          {
> -          switch (p[1])
> +          new_string_pointer[i] = *p;
> +          continue;
> +        }
> +      ++p;
> +
> +      // octal escape sequence

Likewise.

> +      if (isodigit (p[0]))
> +        {
> +          new_string_pointer[i] = p[0] - '0';
> +          if (isodigit (p[1]))
>              {
> -            case '\\': new_string_pointer[i] = '\\'; break;
> -            case 'n':  new_string_pointer[i] = '\n'; break;
> -            case 't':  new_string_pointer[i] = '\t'; break;
> -            case '"':  new_string_pointer[i] = '"'; break;
> -            default:
> -              assert (0);
> +              new_string_pointer[i]
> +                  = (new_string_pointer[i] << 3) | (p[1] - '0');

Care to explain this magic in a comment? :)

> +              ++p;
> +              if (isodigit (p[1]) && p[-1] <= '3')
> +                {
> +                  new_string_pointer[i]
> +                      = (new_string_pointer[i] << 3) | (p[1] - '0');
> +                  ++p;
> +                }
>              }
> -          p++;
> +          if (new_string_pointer[i] == '\0')
> +            {
> +              PKL_ERROR (PKL_AST_LOC (string),
> +                         _ ("string literal cannot contain NULL character"));
> +              PKL_TRANS_PAYLOAD->errors++;
> +              PKL_PASS_ERROR;
> +            }
> +          continue;
> +        }
> +
> +      switch (*p)
> +        {
> +        case '\\': new_string_pointer[i] = '\\'; break;
> +        case 'n': new_string_pointer[i] = '\n'; break;
> +        case 't': new_string_pointer[i] = '\t'; break;
> +        case '"': new_string_pointer[i] = '"'; break;
> +        case 'x':
> +          ++p;
> +          new_string_pointer[i] = xdigit (p[0]);
> +          if (isxdigit (p[1]))
> +            {
> +              new_string_pointer[i] = (xdigit (p[0]) << 4) | xdigit (p[1]);
> +              ++p;
> +            }
> +          if (new_string_pointer[i] == '\0')
> +            {
> +              PKL_ERROR (PKL_AST_LOC (string),
> +                         _ ("string literal cannot contain NULL character"));
> +              PKL_TRANS_PAYLOAD->errors++;
> +              PKL_PASS_ERROR;
> +            }
> +          break;
> +        default:
> +          assert (0);
>          }
> -      else
> -        new_string_pointer[i] = p[0];
>      }
>    new_string_pointer[i] = '\0';
>  
> +#undef isodigit
> +#undef xdigit
> +
>    free (string_pointer);
>    PKL_AST_STRING_POINTER (string) = new_string_pointer;
>  }
> diff --git a/testsuite/Makefile.am b/testsuite/Makefile.am
> index 5ba9900a..20d93ce5 100644
> --- a/testsuite/Makefile.am
> +++ b/testsuite/Makefile.am
> @@ -1370,6 +1370,11 @@ EXTRA_DIST = \
>    poke.pkl/strings-2.pk \
>    poke.pkl/strings-3.pk \
>    poke.pkl/strings-4.pk \
> +  poke.pkl/strings-esc-1.pk \
> +  poke.pkl/strings-esc-2.pk \
> +  poke.pkl/strings-esc-diag-1.pk \
> +  poke.pkl/strings-esc-diag-2.pk \
> +  poke.pkl/strings-esc-diag-3.pk \
>    poke.pkl/strings-index-1.pk \
>    poke.pkl/strings-index-2.pk \
>    poke.pkl/strings-index-diag-1.pk \
> diff --git a/testsuite/poke.pkl/string-diag-1.pk 
> b/testsuite/poke.pkl/string-diag-1.pk
> index 877bf6ad..1daf5c4e 100644
> --- a/testsuite/poke.pkl/string-diag-1.pk
> +++ b/testsuite/poke.pkl/string-diag-1.pk
> @@ -2,4 +2,4 @@
>  
>  /* Invalid \-sequence in string.  */
>  
> -defvar s = "foo\xbar"; /* { dg-error "" } */
> +defvar s = "foo\zbar"; /* { dg-error "" } */
> diff --git a/testsuite/poke.pkl/strings-esc-1.pk 
> b/testsuite/poke.pkl/strings-esc-1.pk
> new file mode 100644
> index 00000000..acb53b8d
> --- /dev/null
> +++ b/testsuite/poke.pkl/strings-esc-1.pk
> @@ -0,0 +1,22 @@
> +/* { dg-do run } */
> +
> +/* Octal escape sequence */
> +
> +defvar s = "\1\12\1234";
> +
> +/* { dg-command { .set obase 8 } } */
> +
> +/* { dg-command { s'length } } */
> +/* { dg-output "0o4UL" } */
> +
> +/* { dg-command {s[0]} } */
> +/* { dg-output "\n0o1UB" } */
> +
> +/* { dg-command {s[1]} } */
> +/* { dg-output "\n0o12UB" } */
> +
> +/* { dg-command {s[2]} } */
> +/* { dg-output "\n0o123UB" } */
> +
> +/* { dg-command {s[3]} } */
> +/* { dg-output "\n0o64UB" } */
> diff --git a/testsuite/poke.pkl/strings-esc-2.pk 
> b/testsuite/poke.pkl/strings-esc-2.pk
> new file mode 100644
> index 00000000..e6ac6591
> --- /dev/null
> +++ b/testsuite/poke.pkl/strings-esc-2.pk
> @@ -0,0 +1,22 @@
> +/* { dg-do run } */
> +
> +/* Hexadecimal escape sequence */
> +
> +defvar s = "\x1\x12\x123";
> +
> +/* { dg-command {.set obase 16} } */
> +
> +/* { dg-command {s'length} } */
> +/* { dg-output "0x4UL" } */
> +
> +/* { dg-command {s[0]} } */
> +/* { dg-output "\n0x1UB" } */
> +
> +/* { dg-command {s[1]} } */
> +/* { dg-output "\n0x12UB" } */
> +
> +/* { dg-command {s[2]} } */
> +/* { dg-output "\n0x12UB" } */
> +
> +/* { dg-command {s[3]} } */
> +/* { dg-output "\n0x33UB" } */
> diff --git a/testsuite/poke.pkl/strings-esc-diag-1.pk 
> b/testsuite/poke.pkl/strings-esc-diag-1.pk
> new file mode 100644
> index 00000000..c51cbccd
> --- /dev/null
> +++ b/testsuite/poke.pkl/strings-esc-diag-1.pk
> @@ -0,0 +1,3 @@
> +/* { dg-do compile } */
> +
> +defvar s = "a\0b"; /* { dg-error "" } */
> diff --git a/testsuite/poke.pkl/strings-esc-diag-2.pk 
> b/testsuite/poke.pkl/strings-esc-diag-2.pk
> new file mode 100644
> index 00000000..f4934e36
> --- /dev/null
> +++ b/testsuite/poke.pkl/strings-esc-diag-2.pk
> @@ -0,0 +1,3 @@
> +/* { dg-do compile } */
> +
> +defvar s = "x\x0z"; /* { dg-error "" } */
> diff --git a/testsuite/poke.pkl/strings-esc-diag-3.pk 
> b/testsuite/poke.pkl/strings-esc-diag-3.pk
> new file mode 100644
> index 00000000..96f5a513
> --- /dev/null
> +++ b/testsuite/poke.pkl/strings-esc-diag-3.pk
> @@ -0,0 +1,5 @@
> +/* { dg-do compile } */
> +
> +/* Invalid \-sequence */
> +
> +defvar s = "a\8b"; /* { dg-error "" } */