From: Jani Nikula <jani@nikula.org>
To: Peter Feigl <craven@gmx.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/3] Adding an S-expression structured output printer.
In-Reply-To: <1354264143-30173-1-git-send-email-craven@gmx.net>
References: <1354264143-30173-1-git-send-email-craven@gmx.net>
User-Agent: Notmuch/0.14+124~g3b17402 (http://notmuchmail.org) Emacs/23.4.1
	(i686-pc-linux-gnu)
Date: Fri, 30 Nov 2012 19:26:04 +0200
Message-ID: <87a9tzj93n.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi Peter, looks good save for a few nitpicks (see comments
inline). Someone more experienced in lisp should still have a look.

BR,
Jani.


On Fri, 30 Nov 2012, Peter Feigl <craven@gmx.net> wrote:
> This commit adds an sprinter for Lisp S-Expressions. Later commits will
> use this printer.
>
> The structure is the same as json, but:
> - arrays are written as lists: ("foo" "bar" "baaz" 1 2 3)
> - maps are written as a-lists: ((key "value") (other-key "other-value"))
> - true is written as t
> - false is written as nil
> - null is written as nil
> ---
>  Makefile.local  |   1 +
>  sprinter-sexp.c | 235 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 236 insertions(+)
>  create mode 100644 sprinter-sexp.c
>
> diff --git a/Makefile.local b/Makefile.local
> index 2b91946..0db1713 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -270,6 +270,7 @@ notmuch_client_srcs =		\
>  	notmuch-tag.c		\
>  	notmuch-time.c		\
>  	sprinter-json.c		\
> +	sprinter-sexp.c		\
>  	sprinter-text.c		\
>  	query-string.c		\
>  	mime-node.c		\
> diff --git a/sprinter-sexp.c b/sprinter-sexp.c
> new file mode 100644
> index 0000000..8401c52
> --- /dev/null
> +++ b/sprinter-sexp.c
> @@ -0,0 +1,235 @@

Copyright comment is missing. (Seems to be missing from sprinter-json.c
too, but that's no excuse! ;)

> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <talloc.h>
> +#include "sprinter.h"
> +
> +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;
> +
> +    /* A flag to signify that a separator should be inserted in the
> +     * output as soon as possible.
> +     */
> +    notmuch_bool_t insert_separator;
> +};
> +
> +struct sexp_state {
> +    struct sexp_state *parent;
> +
> +    /* True if nothing has been printed in this aggregate yet.
> +     * Suppresses the space before a value. */
> +    notmuch_bool_t first;
> +
> +    /* True if the state is a map state.
> +       Used to add a space between key/value pairs. */

The multi-line comments should use the same style here and elsewhere.

> +    notmuch_bool_t in_map;
> +
> +    /* The character that closes the current aggregate. */
> +    char close;
> +};
> +
> +/* Helper function to set up the stream to print a value.  If this
> + * value follows another value, prints a space. */
> +static struct sprinter_sexp *
> +sexp_begin_value (struct sprinter *sp)
> +{
> +    struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
> +
> +    if (sps->state) {
> +        if (! sps->state->first) {
> +            if (sps->insert_separator) {
> +                fputc ('\n', sps->stream);
> +                sps->insert_separator = FALSE;
> +            } else {
> +                if( ! sps->state->in_map)

Spacing should be "if (! " ...

> +                    fputc (' ', sps->stream);
> +            }
> +        } else {
> +            sps->state->first = FALSE;
> +        }
> +    }
> +    return sps;
> +}
> +
> +/* Helper function to begin an aggregate type.  Prints the open
> + * character and pushes a new state frame. */
> +static void
> +sexp_begin_aggregate (struct sprinter *sp, char open, char close)
> +{
> +    struct sprinter_sexp *sps = sexp_begin_value (sp);
> +    struct sexp_state *state = talloc (sps, struct sexp_state);
> +    fputc (open, sps->stream);
> +    state->parent = sps->state;
> +    state->first = TRUE;
> +    state->in_map = FALSE;
> +    state->close = close;
> +    sps->state = state;
> +}
> +
> +static void
> +sexp_begin_map (struct sprinter *sp)
> +{
> +    struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
> +    sexp_begin_aggregate (sp, '(', ')');
> +    sps->state->in_map = TRUE;
> +}
> +
> +static void
> +sexp_begin_list (struct sprinter *sp)
> +{
> +    sexp_begin_aggregate (sp, '(', ')');
> +}
> +
> +static void
> +sexp_end (struct sprinter *sp)
> +{
> +    struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
> +    struct sexp_state *state = sps->state;
> +
> +    if (sps->state->in_map)
> +        fputc (')', sps->stream);
> +    fputc (sps->state->close, sps->stream);
> +    sps->state = state->parent;
> +    talloc_free (state);
> +    if (sps->state == NULL)
> +        fputc ('\n', sps->stream);
> +}
> +
> +/* This implementation supports embedded NULs as allowed by the JSON
> + * specification and Unicode.  Support for *parsing* embedded NULs
> + * varies, but is generally not a problem outside of C-based parsers
> + * (Python's json module and Emacs' json.el take embedded NULs in
> + * stride). */

References to JSON seem out of place here.

> +static void
> +sexp_string_len_internal (struct sprinter *sp, const char *val, size_t len, notmuch_bool_t quote)
> +{
> +    static const char *const escapes[] = {
> +        ['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",
> +        ['\f'] = "\\f",  ['\n'] = "\\n",  ['\t'] = "\\t"
> +    };
> +    struct sprinter_sexp *sps = sexp_begin_value (sp);
> +
> +    if(quote)
> +        fputc ('"', sps->stream);
> +    for (; len; ++val, --len) {
> +        unsigned char ch = *val;
> +        if (ch < ARRAY_SIZE (escapes) && escapes[ch])
> +            fputs (escapes[ch], sps->stream);
> +        else if (ch >= 32)
> +            fputc (ch, sps->stream);
> +        else
> +            fprintf (sps->stream, "\\u%04x", ch);
> +    }
> +    if(quote)
> +        fputc ('"', sps->stream);
> +}
> +
> +static void
> +sexp_string_len (struct sprinter *sp, const char *val, size_t len)
> +{
> +    sexp_string_len_internal (sp, val, len, TRUE); /* print quoted */
> +}
> +
> +static void
> +sexp_symbol_len (struct sprinter *sp, const char *val, size_t len)
> +{
> +    sexp_string_len_internal (sp, val, len, FALSE); /* print unquoted */
> +}
> +
> +static void
> +sexp_string (struct sprinter *sp, const char *val)
> +{
> +    if (val == NULL)
> +        val = "";
> +    sexp_string_len (sp, val, strlen (val));
> +}
> +
> +static void
> +sexp_symbol (struct sprinter *sp, const char *val)
> +{
> +    if (val == NULL)
> +        val = "";
> +    sexp_symbol_len (sp, val, strlen (val));
> +}
> +
> +static void
> +sexp_integer (struct sprinter *sp, int val)
> +{
> +    struct sprinter_sexp *sps = sexp_begin_value (sp);
> +
> +    fprintf (sps->stream, "%d", val);
> +}
> +
> +static void
> +sexp_boolean (struct sprinter *sp, notmuch_bool_t val)
> +{
> +    struct sprinter_sexp *sps = sexp_begin_value (sp);
> +
> +    fputs (val ? "t" : "nil", sps->stream);
> +}
> +
> +static void
> +sexp_null (struct sprinter *sp)
> +{
> +    struct sprinter_sexp *sps = sexp_begin_value (sp);
> +
> +    fputs ("nil", sps->stream);
> +}
> +
> +static void
> +sexp_map_key (struct sprinter *sp, const char *key)
> +{
> +    struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
> +
> +    if( sps->state->in_map && ! sps->state->first)

Spacing should be "if (sps" ...

> +        fputs (") ", sps->stream);
> +    fputc ('(', sps->stream);
> +    sexp_symbol (sp, key);
> +    fputc (' ', sps->stream);
> +}
> +
> +static void
> +sexp_set_prefix (unused (struct sprinter *sp), unused (const char *name))
> +{
> +}
> +
> +static void
> +sexp_separator (struct sprinter *sp)
> +{
> +    struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
> +
> +    sps->insert_separator = TRUE;
> +}
> +
> +struct sprinter *
> +sprinter_sexp_create (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,
> +            .string_len = sexp_string_len,
> +            .integer = sexp_integer,
> +            .boolean = sexp_boolean,
> +            .null = sexp_null,
> +            .map_key = sexp_map_key,
> +            .separator = sexp_separator,
> +            .set_prefix = sexp_set_prefix,
> +            .is_text_printer = FALSE,
> +        }
> +    };
> +    struct sprinter_sexp *res;
> +
> +    res = talloc (ctx, struct sprinter_sexp);
> +    if (! res)
> +        return NULL;
> +
> +    *res = template;
> +    res->stream = stream;
> +    return &res->vtable;
> +}
> -- 
> 1.8.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch