[m-rev.] for review: escape characters in strings returned by deconstruct.functor/4

[Zoltan Somogyi]

On Wed, 11 Jul 2018 00:56:50 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
> diff --git a/NEWS b/NEWS
> index cb97697..3fdc989 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -525,6 +525,10 @@ Changes to the Mercury standard library:
>     - get_uint8/1, unsafe_get_uint8/1
>     - set_uint8/4, unsafe_set_uint8/4
> 
> +* The functor/4 predicate in the deconstruct module has been modified so
> +  that for character and string data any control characters in the functor
> +  representation are escaped.

Add a comma before "any".

> diff --git a/runtime/mercury_string.c b/runtime/mercury_string.c
> index 4c11f3f..a061509 100644
> --- a/runtime/mercury_string.c
> +++ b/runtime/mercury_string.c
> @@ -91,6 +91,129 @@ MR_make_string(MR_AllocSiteInfoPtr alloc_id, const char *fmt, ...)
>       return result;
>   }
> 
> +// The code for this function should be kept in sync with that of the
> +// quote_string predicates in library/term_io.m.
> +MR_bool
> +MR_escape_string_quote(MR_String *ptr, const char * string)
> +{
> +    MR_Integer pos = 0;
> +    size_t  num_code_units = 0;
> +    MR_Char ch;
> +    MR_bool must_escape = MR_FALSE;
> +
> +    // Check if we need to add character escapes to the string.
> +    //
> +    while ((ch = MR_utf8_get_next((MR_String)string, &pos)) > 0) {

Space after cast.

> +    // Check that there the string's encoding was valid.
> +    //

"there"?

And why the extra empty comment line?

> +    if (ch < 0) {
> +        *ptr = NULL;
> +        return MR_FALSE;
> +    }
> +
> +    if (must_escape) {
> +

Delete blank line.

> +        while ((ch = MR_utf8_get_next((MR_String)string, &pos)) > 0) {

Space after cast.

> +// True if c is a Unicode control code point, i.e. U+0000..U+001f,
> +// U+007f..U+009f.
> +
> +#define MR_is_control(c) ((0x00 <= (unsigned)(c) && (unsigned)(c) <= 0x1f) || \
> +                          (0x7f <= (unsigned)(c) && (unsigned)(c) <= 0x9f))
> +

Space after casts.

> diff --git a/tests/hard_coded/deconstruct_arg.exp b/tests/hard_coded/deconstruct_arg.exp
> index 7f4ef4f..49ed6e9 100644
> --- a/tests/hard_coded/deconstruct_arg.exp
> +++ b/tests/hard_coded/deconstruct_arg.exp
> @@ -376,6 +376,104 @@ deconstruct deconstruct: functor 'Ω' arity 0
>   deconstruct limited deconstruct 3 of 'Ω'
>   functor 'Ω' arity 0 []
> 
> +deconstruct functor: ""/0
> +deconstruct argument 0 of "" doesn't exist
> +deconstruct argument 1 of "" doesn't exist
> +deconstruct argument 2 of "" doesn't exist
> +deconstruct argument 'moo' doesn't exist
> +deconstruct argument 'mooo!' doesn't exist
> +deconstruct argument 'packed1' doesn't exist
> +deconstruct argument 'packed2' doesn't exist
> +deconstruct argument 'packed3' doesn't exist
> +deconstruct deconstruct: functor "" arity 0
> +[]
> +deconstruct limited deconstruct 3 of ""
> +functor "" arity 0 []

Before the next change to this test case, we should make its output
less verbose by reporting simply "no argument exists" if all the
argument lookup tests fail for an arity-zero term.

Apart from the minor issues above, the diff seems fine.

Zoltan.