From 8d1c48226cacf3ad3d70bf404cdb22ac22308166 Mon Sep 17 00:00:00 2001 From: Tomi Ollila Date: Thu, 12 Jul 2012 15:05:14 +0300 Subject: [PATCH] Re: [PATCH v4 1/3] Add support for structured output formatters. --- e0/bf1a1a6192a2ac2b7bb5b5466a31ec07edccfb | 383 ++++++++++++++++++++++ 1 file changed, 383 insertions(+) create mode 100644 e0/bf1a1a6192a2ac2b7bb5b5466a31ec07edccfb diff --git a/e0/bf1a1a6192a2ac2b7bb5b5466a31ec07edccfb b/e0/bf1a1a6192a2ac2b7bb5b5466a31ec07edccfb new file mode 100644 index 000000000..67af212e7 --- /dev/null +++ b/e0/bf1a1a6192a2ac2b7bb5b5466a31ec07edccfb @@ -0,0 +1,383 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 4B4B1429E47 + for ; Thu, 12 Jul 2012 05:05:05 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: 0 +X-Spam-Level: +X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] + autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id W87MHVdWr7vF for ; + Thu, 12 Jul 2012 05:05:03 -0700 (PDT) +Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) + by olra.theworths.org (Postfix) with ESMTP id CC832429E25 + for ; Thu, 12 Jul 2012 05:05:02 -0700 (PDT) +Received: by guru.guru-group.fi (Postfix, from userid 501) + id 376E3100386; Thu, 12 Jul 2012 15:05:14 +0300 (EEST) +From: Tomi Ollila +To: Mark Walters , craven@gmx.net, + notmuch@notmuchmail.org +Subject: Re: [PATCH v4 1/3] Add support for structured output formatters. +In-Reply-To: <87liipte5m.fsf@qmul.ac.uk> +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> + <87liipte5m.fsf@qmul.ac.uk> +User-Agent: Notmuch/0.13.2+74~g65b26b0 (http://notmuchmail.org) Emacs/23.1.1 + (x86_64-redhat-linux-gnu) +X-Face: HhBM'cA~ +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Thu, 12 Jul 2012 12:05:05 -0000 + +On Thu, Jul 12 2012, Mark Walters wrote: + +> 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. + +I like the current implementation -- record what has been opened and +have a common closing function which knows what it is closing. Less +checking in the code (i.e. less possible branches). +This makes the use of the interface a bit less self-documenting as +'end' is used instead of list/hash end (defining macros for this +documentation purposes would be really dumb thing to do ;) + +What needs to be checked is that software doesn't attempt to 'end' +too many contexts (i quess it's doing it already). Any other output +errors (like forgetting to 'end' some blocks should be taken care +by proper test cases). + +>>> 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. + +I like that -- is there (currently) need for splinter.c for common code ? +(splinter.h is definitely needed). + +> (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 was thinking the same when looking splinter code yesterday -- how to +have even more common code for json&sexp. Maybe there could be something +to do just for these 2 purposes but It requires more effort and might +add more complexity for humans to perceive. ATM I'd go with this interface +and see later if anyone wants to do/experiment more -- as you said the +risk of diverging is minimal -- and in case there are separate source +files for json & sexp diffing those will be easy. + + +> 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 + +Tomi + + +> +> +> +>> +>> Thanks for the suggestions! +>> +>> Peter +>> From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001 +>> From: +>> 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 +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch -- 2.26.2