e0/bf1a1a6192a2ac2b7bb5b5466a31ec07edccfb

   1 Return-Path: <tomi.ollila@iki.fi>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 4B4B1429E47\r
   6         for <notmuch@notmuchmail.org>; Thu, 12 Jul 2012 05:05:05 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: 0\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none]\r
  12         autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id W87MHVdWr7vF for <notmuch@notmuchmail.org>;\r
  16         Thu, 12 Jul 2012 05:05:03 -0700 (PDT)\r
  17 Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34])\r
  18         by olra.theworths.org (Postfix) with ESMTP id CC832429E25\r
  19         for <notmuch@notmuchmail.org>; Thu, 12 Jul 2012 05:05:02 -0700 (PDT)\r
  20 Received: by guru.guru-group.fi (Postfix, from userid 501)\r
  21         id 376E3100386; Thu, 12 Jul 2012 15:05:14 +0300 (EEST)\r
  22 From: Tomi Ollila <tomi.ollila@iki.fi>\r
  23 To: Mark Walters <markwalters1009@gmail.com>, craven@gmx.net,\r
  24         notmuch@notmuchmail.org\r
  25 Subject: Re: [PATCH v4 1/3] Add support for structured output formatters.\r
  26 In-Reply-To: <87liipte5m.fsf@qmul.ac.uk>\r
  27 References: <87d34hsdx8.fsf@awakening.csail.mit.edu>\r
  28         <1342079004-5300-1-git-send-email-craven@gmx.net>\r
  29         <1342079004-5300-2-git-send-email-craven@gmx.net>\r
  30         <87pq81tjv4.fsf@qmul.ac.uk> <87a9z5teaj.fsf@nexoid.at>\r
  31         <87liipte5m.fsf@qmul.ac.uk>\r
  32 User-Agent: Notmuch/0.13.2+74~g65b26b0 (http://notmuchmail.org) Emacs/23.1.1\r
  33         (x86_64-redhat-linux-gnu)\r
  34 X-Face: HhBM'cA~<r"^Xv\KRN0P{vn'Y"Kd;zg_y3S[4)KSN~s?O\"QPoL\r
  35         $[Xv_BD:i/F$WiEWax}R(MPS`^UaptOGD`*/=@\1lKoVa9tnrg0TW?"r7aRtgk[F\r
  36         !)g;OY^,BjTbr)Np:%c_o'jj,Z\r
  37 Date: Thu, 12 Jul 2012 15:05:14 +0300\r
  38 Message-ID: <m2pq81tcat.fsf@guru.guru-group.fi>\r
  39 MIME-Version: 1.0\r
  40 Content-Type: text/plain; charset=us-ascii\r
  41 X-BeenThere: notmuch@notmuchmail.org\r
  42 X-Mailman-Version: 2.1.13\r
  43 Precedence: list\r
  44 List-Id: "Use and development of the notmuch mail system."\r
  45         <notmuch.notmuchmail.org>\r
  46 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  47         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  48 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  49 List-Post: <mailto:notmuch@notmuchmail.org>\r
  50 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  51 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  52         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  53 X-List-Received-Date: Thu, 12 Jul 2012 12:05:05 -0000\r
  54 \r
  55 On Thu, Jul 12 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
  56 \r
  57 > On Thu, 12 Jul 2012, craven@gmx.net wrote:\r
  58 >>> what is the advantage of having this as one function rather than end_map\r
  59 >>> and end_list? Indeed, my choice (but I think most other people would\r
  60 >>> disagree) would be to have both functions but still keep state as this\r
  61 >>> currently does and then throw an error if the code closes the wrong\r
  62 >>> thing.\r
  63 >>\r
  64 >> There's probably no advantage, one way or the other is fine, I'd say.\r
  65 >> I've thought about introducing checks into the formatter functions, to\r
  66 >> raise errors for improper closing, map_key not inside a map and things\r
  67 >> like that, I just wasn't sure that would be acceptable.\r
  68 >\r
  69 > I will leave others to comment.\r
  70 \r
  71 I like the current implementation -- record what has been opened and\r
  72 have a common closing function which knows what it is closing. Less\r
  73 checking in the code (i.e. less possible branches).\r
  74 This makes the use of the interface a bit less self-documenting as\r
  75 'end' is used instead of list/hash end (defining macros for this \r
  76 documentation purposes would be really dumb thing to do ;)\r
  77 \r
  78 What needs to be checked is that software doesn't attempt to 'end'\r
  79 too many contexts (i quess it's doing it already). Any other output\r
  80 errors (like forgetting to 'end' some blocks should be taken care\r
  81 by proper test cases).\r
  82 \r
  83 >>> A second question: do you have an implementation in this style for\r
  84 >>> s-expressions. I find it hard to tell whether the interface is right\r
  85 >>> with just a single example. Even a completely hacky not ready for review\r
  86 >>> example would be helpful.\r
  87 >>\r
  88 >> See the attached patch :)\r
  89 >\r
  90 > This looks great. I found it much easier to review by splitting\r
  91 > sprinter.c into two files sprinter-json.c and sprinter-sexp.c and then\r
  92 > running meld on them. The similarity is then very clear. It might be\r
  93 > worth submitting them as two files, but I leave other people to comment.\r
  94 \r
  95 I like that -- is there (currently) need for splinter.c for common code ?\r
  96 (splinter.h is definitely needed).\r
  97 \r
  98 > (Doing so made some of the difference between json and s-exp clear: like\r
  99 > that keys in mapkeys are quoted in json but not in s-exp)\r
 100 >\r
 101 > It could be that some of the code could be merged, but I am not sure\r
 102 > that there is much advantage. I would imagine that these two sprinter.c\r
 103 > files would basically never change so there is not much risk of them\r
 104 > diverging.\r
 105 \r
 106 I was thinking the same when looking splinter code yesterday -- how to\r
 107 have even more common code for json&sexp. Maybe there could be something\r
 108 to do just for these 2 purposes but It requires more effort and might\r
 109 add more complexity for humans to perceive. ATM I'd go with this interface\r
 110 and see later if anyone wants to do/experiment more -- as you said the\r
 111 risk of diverging is minimal -- and in case there are separate source\r
 112 files for json & sexp diffing those will be easy.\r
 113 \r
 114 \r
 115 > I wonder if it would be worth using aggregate_t for both rather than\r
 116 > using the closing symbol for this purpose in the json output.\r
 117 >\r
 118 > In any case this patch answers my query: the new structure does\r
 119 > generalise very easily to s-expressions!\r
 120 >\r
 121 > Best wishes\r
 122 >\r
 123 > Mark\r
 124 \r
 125 Tomi\r
 126 \r
 127 \r
 128 >\r
 129 >\r
 130 >\r
 131 >>\r
 132 >> Thanks for the suggestions!\r
 133 >>\r
 134 >> Peter\r
 135 >> From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001\r
 136 >> From: <craven@gmx.net>\r
 137 >> Date: Thu, 12 Jul 2012 10:17:05 +0200\r
 138 >> Subject: [PATCH] Add an S-Expression output format.\r
 139 >>\r
 140 >> ---\r
 141 >>  notmuch-search.c |   7 ++-\r
 142 >>  sprinter.c       | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\r
 143 >>  sprinter.h       |   4 ++\r
 144 >>  3 files changed, 180 insertions(+), 1 deletion(-)\r
 145 >>\r
 146 >> diff --git a/notmuch-search.c b/notmuch-search.c\r
 147 >> index b853f5f..f8bea9b 100644\r
 148 >> --- a/notmuch-search.c\r
 149 >> +++ b/notmuch-search.c\r
 150 >> @@ -77,6 +77,7 @@ do_search_threads (sprinter_t *format,\r
 151 >>  \r
 152 >>      if (format != sprinter_text) {\r
 153 >>      format->begin_list (format);\r
 154 >> +    format->frame (format);\r
 155 >>      }\r
 156 >>  \r
 157 >>      for (i = 0;\r
 158 >> @@ -380,7 +381,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
 159 >>      int exclude = EXCLUDE_TRUE;\r
 160 >>      unsigned int i;\r
 161 >>  \r
 162 >> -    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }\r
 163 >> +    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT, NOTMUCH_FORMAT_SEXP }\r
 164 >>      format_sel = NOTMUCH_FORMAT_TEXT;\r
 165 >>  \r
 166 >>      notmuch_opt_desc_t options[] = {\r
 167 >> @@ -391,6 +392,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
 168 >>      { NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',\r
 169 >>        (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },\r
 170 >>                                { "text", NOTMUCH_FORMAT_TEXT },\r
 171 >> +                              { "sexp", NOTMUCH_FORMAT_SEXP },\r
 172 >>                                { 0, 0 } } },\r
 173 >>      { NOTMUCH_OPT_KEYWORD, &output, "output", 'o',\r
 174 >>        (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },\r
 175 >> @@ -422,6 +424,9 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
 176 >>      case NOTMUCH_FORMAT_JSON:\r
 177 >>      format = sprinter_json_new (ctx, stdout);\r
 178 >>      break;\r
 179 >> +    case NOTMUCH_FORMAT_SEXP:\r
 180 >> +    format = sprinter_sexp_new (ctx, stdout);\r
 181 >> +        break;\r
 182 >>      }\r
 183 >>  \r
 184 >>      config = notmuch_config_open (ctx, NULL, NULL);\r
 185 >> diff --git a/sprinter.c b/sprinter.c\r
 186 >> index 649f79a..fce0f9b 100644\r
 187 >> --- a/sprinter.c\r
 188 >> +++ b/sprinter.c\r
 189 >> @@ -170,3 +170,173 @@ sprinter_json_new(const void *ctx, FILE *stream)\r
 190 >>      res->stream = stream;\r
 191 >>      return &res->vtable;\r
 192 >>  }\r
 193 >> +\r
 194 >> +/*\r
 195 >> + * Every below here is the implementation of the SEXP printer.\r
 196 >> + */\r
 197 >> +\r
 198 >> +typedef enum { MAP, LIST } aggregate_t;\r
 199 >> +\r
 200 >> +struct sprinter_sexp\r
 201 >> +{\r
 202 >> +    struct sprinter vtable;\r
 203 >> +    FILE *stream;\r
 204 >> +    /* Top of the state stack, or NULL if the printer is not currently\r
 205 >> +     * inside any aggregate types. */\r
 206 >> +    struct sexp_state *state;\r
 207 >> +};\r
 208 >> +\r
 209 >> +struct sexp_state\r
 210 >> +{\r
 211 >> +    struct sexp_state *parent;\r
 212 >> +    /* True if nothing has been printed in this aggregate yet.\r
 213 >> +     * Suppresses the comma before a value. */\r
 214 >> +    notmuch_bool_t first;\r
 215 >> +    /* The character that closes the current aggregate. */\r
 216 >> +    aggregate_t type;\r
 217 >> +};\r
 218 >> +\r
 219 >> +static struct sprinter_sexp *\r
 220 >> +sexp_begin_value(struct sprinter *sp)\r
 221 >> +{\r
 222 >> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;\r
 223 >> +    if (spsx->state) {\r
 224 >> +    if (!spsx->state->first)\r
 225 >> +        fputc (' ', spsx->stream);\r
 226 >> +    else\r
 227 >> +        spsx->state->first = false;\r
 228 >> +    }\r
 229 >> +    return spsx;\r
 230 >> +}\r
 231 >> +\r
 232 >> +static void\r
 233 >> +sexp_begin_aggregate(struct sprinter *sp, aggregate_t type)\r
 234 >> +{\r
 235 >> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;\r
 236 >> +    struct sexp_state *state = talloc (spsx, struct sexp_state);\r
 237 >> +\r
 238 >> +    fputc ('(', spsx->stream);\r
 239 >> +    state->parent = spsx->state;\r
 240 >> +    state->first = true;\r
 241 >> +    spsx->state = state;\r
 242 >> +    state->type = type;\r
 243 >> +}\r
 244 >> +\r
 245 >> +static void\r
 246 >> +sexp_begin_map(struct sprinter *sp)\r
 247 >> +{\r
 248 >> +    sexp_begin_aggregate (sp, MAP);\r
 249 >> +}\r
 250 >> +\r
 251 >> +static void\r
 252 >> +sexp_begin_list(struct sprinter *sp)\r
 253 >> +{\r
 254 >> +    sexp_begin_aggregate (sp, LIST);\r
 255 >> +}\r
 256 >> +\r
 257 >> +static void\r
 258 >> +sexp_end(struct sprinter *sp)\r
 259 >> +{\r
 260 >> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;\r
 261 >> +    struct sexp_state *state = spsx->state;\r
 262 >> +\r
 263 >> +    fputc (')', spsx->stream);\r
 264 >> +    spsx->state = state->parent;\r
 265 >> +    talloc_free (state);\r
 266 >> +    if(spsx->state == NULL)\r
 267 >> +    fputc ('\n', spsx->stream);\r
 268 >> +}\r
 269 >> +\r
 270 >> +static void\r
 271 >> +sexp_string(struct sprinter *sp, const char *val)\r
 272 >> +{\r
 273 >> +    static const char * const escapes[] = {\r
 274 >> +    ['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",\r
 275 >> +    ['\f'] = "\\f",  ['\n'] = "\\n",  ['\t'] = "\\t"\r
 276 >> +    };\r
 277 >> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);\r
 278 >> +    fputc ('"', spsx->stream);\r
 279 >> +    for (; *val; ++val) {\r
 280 >> +    unsigned char ch = *val;\r
 281 >> +    if (ch < ARRAY_SIZE(escapes) && escapes[ch])\r
 282 >> +        fputs (escapes[ch], spsx->stream);\r
 283 >> +    else if (ch >= 32)\r
 284 >> +        fputc (ch, spsx->stream);\r
 285 >> +    else\r
 286 >> +        fprintf (spsx->stream, "\\u%04x", ch);\r
 287 >> +    }\r
 288 >> +    fputc ('"', spsx->stream);\r
 289 >> +    if (spsx->state != NULL &&  spsx->state->type == MAP)\r
 290 >> +    fputc (')', spsx->stream);\r
 291 >> +    spsx->state->first = false;\r
 292 >> +}\r
 293 >> +\r
 294 >> +static void\r
 295 >> +sexp_integer(struct sprinter *sp, int val)\r
 296 >> +{\r
 297 >> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);\r
 298 >> +    fprintf (spsx->stream, "%d", val);\r
 299 >> +    if (spsx->state != NULL &&  spsx->state->type == MAP)\r
 300 >> +    fputc (')', spsx->stream);\r
 301 >> +}\r
 302 >> +\r
 303 >> +static void\r
 304 >> +sexp_boolean(struct sprinter *sp, notmuch_bool_t val)\r
 305 >> +{\r
 306 >> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);\r
 307 >> +    fputs (val ? "#t" : "#f", spsx->stream);\r
 308 >> +    if (spsx->state != NULL &&  spsx->state->type == MAP)\r
 309 >> +    fputc (')', spsx->stream);\r
 310 >> +}\r
 311 >> +\r
 312 >> +static void\r
 313 >> +sexp_null(struct sprinter *sp)\r
 314 >> +{\r
 315 >> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);\r
 316 >> +    fputs ("'()", spsx->stream);\r
 317 >> +    spsx->state->first = false;\r
 318 >> +}\r
 319 >> +\r
 320 >> +static void\r
 321 >> +sexp_map_key(struct sprinter *sp, const char *key)\r
 322 >> +{\r
 323 >> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);\r
 324 >> +    fputc ('(', spsx->stream);\r
 325 >> +    fputs (key, spsx->stream);\r
 326 >> +    fputs (" . ", spsx->stream);\r
 327 >> +    spsx->state->first = true;\r
 328 >> +}\r
 329 >> +\r
 330 >> +static void\r
 331 >> +sexp_frame(struct sprinter *sp)\r
 332 >> +{\r
 333 >> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;\r
 334 >> +    fputc ('\n', spsx->stream);\r
 335 >> +}\r
 336 >> +\r
 337 >> +struct sprinter *\r
 338 >> +sprinter_sexp_new(const void *ctx, FILE *stream)\r
 339 >> +{\r
 340 >> +    static const struct sprinter_sexp template = {\r
 341 >> +    .vtable = {\r
 342 >> +        .begin_map = sexp_begin_map,\r
 343 >> +        .begin_list = sexp_begin_list,\r
 344 >> +        .end = sexp_end,\r
 345 >> +        .string = sexp_string,\r
 346 >> +        .integer = sexp_integer,\r
 347 >> +        .boolean = sexp_boolean,\r
 348 >> +        .null = sexp_null,\r
 349 >> +        .map_key = sexp_map_key,\r
 350 >> +        .frame = sexp_frame,\r
 351 >> +    }\r
 352 >> +    };\r
 353 >> +    struct sprinter_sexp *res;\r
 354 >> +\r
 355 >> +    res = talloc (ctx, struct sprinter_sexp);\r
 356 >> +    if (!res)\r
 357 >> +    return NULL;\r
 358 >> +\r
 359 >> +    *res = template;\r
 360 >> +    res->stream = stream;\r
 361 >> +    return &res->vtable;\r
 362 >> +}\r
 363 >> diff --git a/sprinter.h b/sprinter.h\r
 364 >> index 1dad9a0..a89eaa5 100644\r
 365 >> --- a/sprinter.h\r
 366 >> +++ b/sprinter.h\r
 367 >> @@ -40,6 +40,10 @@ typedef struct sprinter\r
 368 >>  struct sprinter *\r
 369 >>  sprinter_json_new(const void *ctx, FILE *stream);\r
 370 >>  \r
 371 >> +/* Create a new structure printer that emits S-Expressions */\r
 372 >> +struct sprinter *\r
 373 >> +sprinter_sexp_new(const void *ctx, FILE *stream);\r
 374 >> +\r
 375 >>  /* A dummy structure printer that signifies that standard text output is\r
 376 >>   * to be used instead of any structured format.\r
 377 >>   */\r
 378 >> -- \r
 379 >> 1.7.11.1\r
 380 > _______________________________________________\r
 381 > notmuch mailing list\r
 382 > notmuch@notmuchmail.org\r
 383 > http://notmuchmail.org/mailman/listinfo/notmuch\r