From: Mark Walters <markwalters1009@gmail.com>
To: craven@gmx.net, notmuch@notmuchmail.org
Subject: Re: [PATCH v4 1/3] Add support for structured output formatters.
In-Reply-To: <87a9z5teaj.fsf@nexoid.at>
References: <87d34hsdx8.fsf@awakening.csail.mit.edu>
	<1342079004-5300-1-git-send-email-craven@gmx.net>
	<1342079004-5300-2-git-send-email-craven@gmx.net>
	<87pq81tjv4.fsf@qmul.ac.uk> <87a9z5teaj.fsf@nexoid.at>
User-Agent: Notmuch/0.13.2+61~gf708609 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Thu, 12 Jul 2012 12:25:09 +0100
Message-ID: <87liipte5m.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Thu, 12 Jul 2012, craven@gmx.net wrote:
>> what is the advantage of having this as one function rather than end_map
>> and end_list? Indeed, my choice (but I think most other people would
>> disagree) would be to have both functions but still keep state as this
>> currently does and then throw an error if the code closes the wrong
>> thing.
>
> There's probably no advantage, one way or the other is fine, I'd say.
> I've thought about introducing checks into the formatter functions, to
> raise errors for improper closing, map_key not inside a map and things
> like that, I just wasn't sure that would be acceptable.

I will leave others to comment.

>> A second question: do you have an implementation in this style for
>> s-expressions. I find it hard to tell whether the interface is right
>> with just a single example. Even a completely hacky not ready for review
>> example would be helpful.
>
> See the attached patch :)

This looks great. I found it much easier to review by splitting
sprinter.c into two files sprinter-json.c and sprinter-sexp.c and then
running meld on them. The similarity is then very clear. It might be
worth submitting them as two files, but I leave other people to comment.

(Doing so made some of the difference between json and s-exp clear: like
that keys in mapkeys are quoted in json but not in s-exp)

It could be that some of the code could be merged, but I am not sure
that there is much advantage. I would imagine that these two sprinter.c
files would basically never change so there is not much risk of them
diverging.

I wonder if it would be worth using aggregate_t for both rather than
using the closing symbol for this purpose in the json output.

In any case this patch answers my query: the new structure does
generalise very easily to s-expressions!

Best wishes

Mark



>
> Thanks for the suggestions!
>
> Peter
> From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001
> From: <craven@gmx.net>
> Date: Thu, 12 Jul 2012 10:17:05 +0200
> Subject: [PATCH] Add an S-Expression output format.
>
> ---
>  notmuch-search.c |   7 ++-
>  sprinter.c       | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sprinter.h       |   4 ++
>  3 files changed, 180 insertions(+), 1 deletion(-)
>
> diff --git a/notmuch-search.c b/notmuch-search.c
> index b853f5f..f8bea9b 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -77,6 +77,7 @@ do_search_threads (sprinter_t *format,
>  
>      if (format != sprinter_text) {
>  	format->begin_list (format);
> +	format->frame (format);
>      }
>  
>      for (i = 0;
> @@ -380,7 +381,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      int exclude = EXCLUDE_TRUE;
>      unsigned int i;
>  
> -    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
> +    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT, NOTMUCH_FORMAT_SEXP }
>  	format_sel = NOTMUCH_FORMAT_TEXT;
>  
>      notmuch_opt_desc_t options[] = {
> @@ -391,6 +392,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
>  	  (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
>  				  { "text", NOTMUCH_FORMAT_TEXT },
> +				  { "sexp", NOTMUCH_FORMAT_SEXP },
>  				  { 0, 0 } } },
>  	{ NOTMUCH_OPT_KEYWORD, &output, "output", 'o',
>  	  (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
> @@ -422,6 +424,9 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      case NOTMUCH_FORMAT_JSON:
>  	format = sprinter_json_new (ctx, stdout);
>  	break;
> +    case NOTMUCH_FORMAT_SEXP:
> +	format = sprinter_sexp_new (ctx, stdout);
> +        break;
>      }
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
> diff --git a/sprinter.c b/sprinter.c
> index 649f79a..fce0f9b 100644
> --- a/sprinter.c
> +++ b/sprinter.c
> @@ -170,3 +170,173 @@ sprinter_json_new(const void *ctx, FILE *stream)
>      res->stream = stream;
>      return &res->vtable;
>  }
> +
> +/*
> + * Every below here is the implementation of the SEXP printer.
> + */
> +
> +typedef enum { MAP, LIST } aggregate_t;
> +
> +struct sprinter_sexp
> +{
> +    struct sprinter vtable;
> +    FILE *stream;
> +    /* Top of the state stack, or NULL if the printer is not currently
> +     * inside any aggregate types. */
> +    struct sexp_state *state;
> +};
> +
> +struct sexp_state
> +{
> +    struct sexp_state *parent;
> +    /* True if nothing has been printed in this aggregate yet.
> +     * Suppresses the comma before a value. */
> +    notmuch_bool_t first;
> +    /* The character that closes the current aggregate. */
> +    aggregate_t type;
> +};
> +
> +static struct sprinter_sexp *
> +sexp_begin_value(struct sprinter *sp)
> +{
> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
> +    if (spsx->state) {
> +	if (!spsx->state->first)
> +	    fputc (' ', spsx->stream);
> +	else
> +	    spsx->state->first = false;
> +    }
> +    return spsx;
> +}
> +
> +static void
> +sexp_begin_aggregate(struct sprinter *sp, aggregate_t type)
> +{
> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
> +    struct sexp_state *state = talloc (spsx, struct sexp_state);
> +
> +    fputc ('(', spsx->stream);
> +    state->parent = spsx->state;
> +    state->first = true;
> +    spsx->state = state;
> +    state->type = type;
> +}
> +
> +static void
> +sexp_begin_map(struct sprinter *sp)
> +{
> +    sexp_begin_aggregate (sp, MAP);
> +}
> +
> +static void
> +sexp_begin_list(struct sprinter *sp)
> +{
> +    sexp_begin_aggregate (sp, LIST);
> +}
> +
> +static void
> +sexp_end(struct sprinter *sp)
> +{
> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
> +    struct sexp_state *state = spsx->state;
> +
> +    fputc (')', spsx->stream);
> +    spsx->state = state->parent;
> +    talloc_free (state);
> +    if(spsx->state == NULL)
> +	fputc ('\n', spsx->stream);
> +}
> +
> +static void
> +sexp_string(struct sprinter *sp, const char *val)
> +{
> +    static const char * const escapes[] = {
> +	['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",
> +	['\f'] = "\\f",  ['\n'] = "\\n",  ['\t'] = "\\t"
> +    };
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fputc ('"', spsx->stream);
> +    for (; *val; ++val) {
> +	unsigned char ch = *val;
> +	if (ch < ARRAY_SIZE(escapes) && escapes[ch])
> +	    fputs (escapes[ch], spsx->stream);
> +	else if (ch >= 32)
> +	    fputc (ch, spsx->stream);
> +	else
> +	    fprintf (spsx->stream, "\\u%04x", ch);
> +    }
> +    fputc ('"', spsx->stream);
> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
> +	fputc (')', spsx->stream);
> +    spsx->state->first = false;
> +}
> +
> +static void
> +sexp_integer(struct sprinter *sp, int val)
> +{
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fprintf (spsx->stream, "%d", val);
> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
> +	fputc (')', spsx->stream);
> +}
> +
> +static void
> +sexp_boolean(struct sprinter *sp, notmuch_bool_t val)
> +{
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fputs (val ? "#t" : "#f", spsx->stream);
> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
> +	fputc (')', spsx->stream);
> +}
> +
> +static void
> +sexp_null(struct sprinter *sp)
> +{
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fputs ("'()", spsx->stream);
> +    spsx->state->first = false;
> +}
> +
> +static void
> +sexp_map_key(struct sprinter *sp, const char *key)
> +{
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fputc ('(', spsx->stream);
> +    fputs (key, spsx->stream);
> +    fputs (" . ", spsx->stream);
> +    spsx->state->first = true;
> +}
> +
> +static void
> +sexp_frame(struct sprinter *sp)
> +{
> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
> +    fputc ('\n', spsx->stream);
> +}
> +
> +struct sprinter *
> +sprinter_sexp_new(const void *ctx, FILE *stream)
> +{
> +    static const struct sprinter_sexp template = {
> +	.vtable = {
> +	    .begin_map = sexp_begin_map,
> +	    .begin_list = sexp_begin_list,
> +	    .end = sexp_end,
> +	    .string = sexp_string,
> +	    .integer = sexp_integer,
> +	    .boolean = sexp_boolean,
> +	    .null = sexp_null,
> +	    .map_key = sexp_map_key,
> +	    .frame = sexp_frame,
> +	}
> +    };
> +    struct sprinter_sexp *res;
> +
> +    res = talloc (ctx, struct sprinter_sexp);
> +    if (!res)
> +	return NULL;
> +
> +    *res = template;
> +    res->stream = stream;
> +    return &res->vtable;
> +}
> diff --git a/sprinter.h b/sprinter.h
> index 1dad9a0..a89eaa5 100644
> --- a/sprinter.h
> +++ b/sprinter.h
> @@ -40,6 +40,10 @@ typedef struct sprinter
>  struct sprinter *
>  sprinter_json_new(const void *ctx, FILE *stream);
>  
> +/* Create a new structure printer that emits S-Expressions */
> +struct sprinter *
> +sprinter_sexp_new(const void *ctx, FILE *stream);
> +
>  /* A dummy structure printer that signifies that standard text output is
>   * to be used instead of any structured format.
>   */
> -- 
> 1.7.11.1