From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 03/13] sprinter: Add a string_len method
In-Reply-To: <1343183693-17134-4-git-send-email-amdragon@mit.edu>
References: <1343183693-17134-1-git-send-email-amdragon@mit.edu>
	<1343183693-17134-4-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.13.2+93~gf33b188 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Wed, 25 Jul 2012 19:57:07 +0100
Message-ID: <87a9ynis8s.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi

I have read this series (apart from the test changes) and it is both
very nice to review and looks good. It builds and all tests pass at all
the interesting partial stages and when fully applied. I have a few very
minor comments.

On Wed, 25 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This method allows callers to output strings with specific lengths.
> It's useful both for strings with embedded NULs (which JSON can
> represent, though parser support is apparently spotty), and
> non-terminated strings.
> ---
>  sprinter-json.c |   11 +++++++++--
>  sprinter-text.c |   11 +++++++++--
>  sprinter.h      |    1 +
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/sprinter-json.c b/sprinter-json.c
> index 4649655..2587ca6 100644
> --- a/sprinter-json.c
> +++ b/sprinter-json.c
> @@ -89,7 +89,7 @@ json_end (struct sprinter *sp)
>  }
>  
>  static void
> -json_string (struct sprinter *sp, const char *val)
> +json_string_len (struct sprinter *sp, const char *val, size_t len)
>  {

I think this function could do with a comment along the lines of the
commit message. It might be nice to document somewhere where/when we
might actually have nulls in an encoded string (are they allowed in
bodies, headers etc).

Actually, do we know that the json emacs parser handles nulls correctly?

Best wishes

Mark
>      static const char *const escapes[] = {
>  	['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",
> @@ -98,7 +98,7 @@ json_string (struct sprinter *sp, const char *val)
>      struct sprinter_json *spj = json_begin_value (sp);
>  
>      fputc ('"', spj->stream);
> -    for (; *val; ++val) {
> +    for (; len; ++val, --len) {
>  	unsigned char ch = *val;
>  	if (ch < ARRAY_SIZE (escapes) && escapes[ch])
>  	    fputs (escapes[ch], spj->stream);
> @@ -111,6 +111,12 @@ json_string (struct sprinter *sp, const char *val)
>  }
>  
>  static void
> +json_string (struct sprinter *sp, const char *val)
> +{
> +    json_string_len (sp, val, strlen (val));
> +}
> +
> +static void
>  json_integer (struct sprinter *sp, int val)
>  {
>      struct sprinter_json *spj = json_begin_value (sp);
> @@ -166,6 +172,7 @@ sprinter_json_create (const void *ctx, FILE *stream)
>  	    .begin_list = json_begin_list,
>  	    .end = json_end,
>  	    .string = json_string,
> +	    .string_len = json_string_len,
>  	    .integer = json_integer,
>  	    .boolean = json_boolean,
>  	    .null = json_null,
> diff --git a/sprinter-text.c b/sprinter-text.c
> index b208840..dfa54b5 100644
> --- a/sprinter-text.c
> +++ b/sprinter-text.c
> @@ -25,14 +25,20 @@ struct sprinter_text {
>  };
>  
>  static void
> -text_string (struct sprinter *sp, const char *val)
> +text_string_len (struct sprinter *sp, const char *val, size_t len)
>  {
>      struct sprinter_text *sptxt = (struct sprinter_text *) sp;
>  
>      if (sptxt->current_prefix != NULL)
>  	fprintf (sptxt->stream, "%s:", sptxt->current_prefix);
>  
> -    fputs(val, sptxt->stream);
> +    fwrite (val, len, 1, sptxt->stream);
> +}
> +
> +static void
> +text_string (struct sprinter *sp, const char *val)
> +{
> +    text_string_len (sp, val, strlen (val));
>  }
>  
>  static void
> @@ -105,6 +111,7 @@ sprinter_text_create (const void *ctx, FILE *stream)
>  	    .begin_list = text_begin_list,
>  	    .end = text_end,
>  	    .string = text_string,
> +	    .string_len = text_string_len,
>  	    .integer = text_integer,
>  	    .boolean = text_boolean,
>  	    .null = text_null,
> diff --git a/sprinter.h b/sprinter.h
> index 6680d41..826a852 100644
> --- a/sprinter.h
> +++ b/sprinter.h
> @@ -28,6 +28,7 @@ typedef struct sprinter {
>       * For string, the char * must be UTF-8 encoded.
>       */
>      void (*string) (struct sprinter *, const char *);
> +    void (*string_len) (struct sprinter *, const char *, size_t);
>      void (*integer) (struct sprinter *, int);
>      void (*boolean) (struct sprinter *, notmuch_bool_t);
>      void (*null) (struct sprinter *);
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch