From: Mark Walters <markwalters1009@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, Peter Feigl <craven@gmx.net>,
	notmuch@notmuchmail.org
Subject: Re: [PATCH 1/3] Adding an S-expression structured output printer.
In-Reply-To: <m2fw3pq4sf.fsf@guru.guru-group.fi>
References: <1354264143-30173-1-git-send-email-craven@gmx.net>
	<871ufa9jpm.fsf@qmul.ac.uk> <m2txs6vwdy.fsf@guru.guru-group.fi>
	<87d2yu559y.fsf@qmul.ac.uk> <m2fw3pq4sf.fsf@guru.guru-group.fi>
User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 01 Dec 2012 13:45:48 +0000
Message-ID: <874nk5ansj.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Sat, Dec 01 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>
>> On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>>> On Sat, Dec 01 2012, Mark Walters wrote:
>>>
>>>> Hi
>>>>
>>>> Overall I like the series: I think I agree with all of Jani's
>>>> comments. 
>>>>
>>>> My one extra comment is that I think we should decide on whether we also
>>>> want a sexp plist version. I think we might want one for the emacs
>>>> front-end as that currently uses plists for everything.
>>>>
>>>> If we do we might want to change the names a little, both for functions
>>>> and options (eg sexp_a and sexp_p or something). Probably a lot of
>>>> sprinter-sexp would be common to both versions.
>>>
>>> This is an important question that needs to be addressed fast: options
>>> are:
>>>
>>> 1) have options to spit both alist & plist formats
>>> 2) when converting emacs to use s-expressions, convert it to use alists
>>> 3) start using plists instead of alists in Peter's android client
>>
>> Ok I have looked at this and the changes needed to output plist (or
>> both) are pretty small: the only functions from sprinter-sexp.c that
>> need to be changed are sexp_end and sexp_map_key. The total diff from
>> alist to plist is about 10 lines. I have a version which allows both
>> (the same sprinter file creates both possibilities) and have hooked it
>> into emacs/notmuch-show.el and it all seems to work. 
>>
>> (Search is more difficult as that uses the async parser; indeed even for
>> show I used sexp-at-point as suggested by Tomi which seems rather
>> underdocumented but does seem to work)
>>
>> Given the ease with which we can allow both I think that would be my
>> preference: the biggest problem is that slightly more cluttered option
>> list (i.e., we have to allow both --format=sexpa and --format=sexpp or
>> similar).
>>
>> (I can post the patch doing the above but almost all of it is modifying the
>> commands to choose alist or plist rather than modifying the
>> sprinter-sexp itself)
>
> As the diff is so small I agree that supporting 2 formats is good
> option.
>
> In case this is done I suggest that we proceed the following way:
>
> 1) Agree how we call these formats (sexpa & sexpp or something else)
> 2) Peter does his updates, including to call the format as will be decided
> 3) Mark posts his patches after Peter's work is pushed

I think this seems a good approach. I should note that while the diff
between alist and plist is only about 10 lines I do need to duplicate
some of the surrounding code to allow both: its not huge but definitely
more than 10 lines. 

I will clean it up a little and then post (as a diff on top of Peter's
patches) just so people can judge if it looks basically acceptable.

Best wishes 

Mark



>
>>
>> Best wishes
>>
>> Mark
>
> Tomi
>
>
>>
>>
>>
>>
>>
>>>
>>>
>>> In case (1) is chosen then we just need to support one more format.
>>>
>>> How much work would it involve to convert emacs to receive content in
>>> alists (and how feasible would that be)?
>>>
>>> How much work would it require in Peter's client to use plists (and how
>>> feasible would that be)?
>>>
>>>>
>>>> Best wishes
>>>>
>>>> Mark
>>>
>>> Tomi
>>>
>>>
>>>>
>>>>
>>>> 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 @@
>>>>> +#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. */
>>>>> +    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)
>>>>> +                    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). */
>>>>> +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)
>>>>> +        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
>>>> _______________________________________________
>>>> notmuch mailing list
>>>> notmuch@notmuchmail.org
>>>> http://notmuchmail.org/mailman/listinfo/notmuch
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch