From: Jani Nikula <jani@nikula.org>
To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org
Subject: Re: [RFC PATCH] bikeshed uncrustify options
In-Reply-To: <1327504886-8730-1-git-send-email-tomi.ollila@iki.fi>
References: <1327504886-8730-1-git-send-email-tomi.ollila@iki.fi>
User-Agent: Notmuch/0.10.2+187~g43d4f26 (http://notmuchmail.org) Emacs/23.1.1
	(i686-pc-linux-gnu)
Date: Wed, 25 Jan 2012 15:48:03 +0000
Message-ID: <87mx9b6ax8.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Tomi Ollila <tomi.ollila@iki.fi>
Precedence: list


Coding style bikeshedding, wonderful. :)

On Wed, 25 Jan 2012 17:21:26 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> Is (generally) using *INDENT-(OFF|ON)*  ok ? 

Obviously I wish we could do without.

You should cook up a wrapper for uncrustify that takes a patch, applies
it, and checks if it introduces new coding style issues, but ignores the
existing ones. ;)

> Would it be ok to have
> #define STRINGIFY(s) STRINGIFY_ (s)

Why not, it's the notmuch style?

> (now such expansion disabled by INDENT-OFF)
> When used, it is still thought as function call,
> and whitespace added.
> 
> What about enum { } \n format_sel change below ?

See comment inline below.

> After applying this patch and running:
> 
> $ uncrustify --replace -c devel/uncrustify.cfg *.[ch]
> 
> one can discuss (at least):
> 
> * should there be space after '!'

That's like having space in '- foo' or '~ foo', isn't it? I'd prefer no
space after unary operators.

> * should there be space after (cast)

Yes, please.

> 
> ---
>  devel/uncrustify.cfg |    3 ++-
>  notmuch-client.h     |    5 ++++-
>  notmuch-reply.c      |    2 ++
>  notmuch-search.c     |   13 +++++++++----
>  notmuch-show.c       |    8 ++++++++
>  notmuch-time.c       |    3 +++
>  6 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/devel/uncrustify.cfg b/devel/uncrustify.cfg
> index d8075ba..a752fae 100644
> --- a/devel/uncrustify.cfg
> +++ b/devel/uncrustify.cfg
> @@ -58,7 +58,8 @@ nl_after_struct = 0
>  # Extra types used in notmuch source.
>  # (add more on demand)
>  
> -type GMimeObject mime_node_t
> +type GMimeObject GMimeCryptoContext GMimeCipherContext
> +type mime_node_t notmuch_message_t
>  
>  #
>  # inter-character spacing options
> diff --git a/notmuch-client.h b/notmuch-client.h
> index e0eb594..131e453 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -59,8 +59,10 @@
>  
>  #define unused(x) x __attribute__ ((unused))
>  
> +/* *INDENT-OFF* */
>  #define STRINGIFY(s) STRINGIFY_(s)
>  #define STRINGIFY_(s) #s
> +/* *INDENT-ON* */
>  
>  struct mime_node;
>  struct notmuch_show_params;
> @@ -377,7 +379,8 @@ mime_node_t *
>  mime_node_child (mime_node_t *parent, int child);
>  
>  /* Return the nth child of node in a depth-first traversal.  If n is
> - * 0, returns node itself.  Returns NULL if there is no such part. */
> + * 0, returns node itself.  Returns NULL if there is no such part.
> + */
>  mime_node_t *
>  mime_node_seek_dfs (mime_node_t *node, int n);
>  
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index f55b1d2..57742c4 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -31,6 +31,7 @@ static void
>  reply_part_content (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_reply = {
> +/* *INDENT-OFF* */

What does it do here without the comment?

>      "", NULL,
>  	"", NULL,
>  	    "", NULL, reply_headers_message_part, ">\n",
> @@ -44,6 +45,7 @@ static const notmuch_show_format_t format_reply = {
>  	    "",
>  	"", "",
>      ""
> +/* *INDENT-ON* */
>  };
>  
>  static void
> diff --git a/notmuch-search.c b/notmuch-search.c
> index d504051..57ec603 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -65,6 +65,7 @@ format_thread_text (const void *ctx,
>  		    const char *authors,
>  		    const char *subject);
>  static const search_format_t format_text = {
> +/* *INDENT-OFF* */
>      "",
>  	"",
>  	    format_item_id_text,
> @@ -75,6 +76,7 @@ static const search_format_t format_text = {
>  	"",
>      "\n",
>      "",
> +/* *INDENT-ON* */
>  };
>  
>  static void
> @@ -91,6 +93,7 @@ format_thread_json (const void *ctx,
>  		    const char *authors,
>  		    const char *subject);
>  static const search_format_t format_json = {
> +/* *INDENT-OFF* */
>      "[",
>  	"{",
>  	    format_item_id_json,
> @@ -101,6 +104,7 @@ static const search_format_t format_json = {
>  	"}",
>      "]\n",
>      "]\n",
> +/* *INDENT-ON* */
>  };
>  
>  static void
> @@ -160,7 +164,7 @@ format_item_id_json (const void *ctx,
>      printf ("%s", json_quote_str (ctx_quote, item_id));
>  
>      talloc_free (ctx_quote);
> -    
> +
>  }
>  
>  static void
> @@ -333,7 +337,7 @@ do_search_messages (const search_format_t *format,
>  
>  		first_message = 0;
>  	    }
> -	    
> +
>  	    notmuch_filenames_destroy( filenames );
>  
>  	} else { /* output == OUTPUT_MESSAGES */
> @@ -427,8 +431,9 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      size_t search_exclude_tags_length;
>      unsigned int i;
>  
> -    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
> -	format_sel = NOTMUCH_FORMAT_TEXT;
> +    enum { /* note: also emacs indents this wrongly if not like this. */
> +	NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT
> +    } format_sel = NOTMUCH_FORMAT_TEXT;

I prefer this style anyway. Compare with:

	struct { int foo; int bar; } baz;

which I think would be frowned upon.

>  
>      notmuch_opt_desc_t options[] = {
>  	{ NOTMUCH_OPT_KEYWORD, &sort, "sort", 's',
> diff --git a/notmuch-show.c b/notmuch-show.c
> index dec799c..dfe37bc 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -42,6 +42,7 @@ static void
>  format_part_end_text (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_text = {
> +    /* *INDENT-OFF* */
>      "", NULL,
>  	"\fmessage{ ", format_message_text,
>  	    "\fheader{\n", format_headers_text, format_headers_message_part_text, "\fheader}\n",
> @@ -55,6 +56,7 @@ static const notmuch_show_format_t format_text = {
>  	    "\fbody}\n",
>  	"\fmessage}\n", "",
>      ""
> +    /* *INDENT-ON* */
>  };
>  
>  static void
> @@ -89,6 +91,7 @@ static void
>  format_part_end_json (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_json = {
> +    /* *INDENT-OFF* */
>      "[", NULL,
>  	"{", format_message_json,
>  	    "\"headers\": {", format_headers_json, format_headers_message_part_json, "}",
> @@ -102,6 +105,7 @@ static const notmuch_show_format_t format_json = {
>  	    "]",
>  	"}", ", ",
>      "]"
> +    /* *INDENT-ON* */
>  };
>  
>  static void
> @@ -110,6 +114,7 @@ format_message_mbox (const void *ctx,
>  		     unused (int indent));
>  
>  static const notmuch_show_format_t format_mbox = {
> +    /* *INDENT-OFF* */
>      "", NULL,
>          "", format_message_mbox,
>              "", NULL, NULL, "",
> @@ -123,12 +128,14 @@ static const notmuch_show_format_t format_mbox = {
>              "",
>          "", "",
>      ""
> +    /* *INDENT-ON* */
>  };
>  
>  static void
>  format_part_content_raw (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_raw = {
> +    /* *INDENT-OFF* */
>      "", NULL,
>  	"", NULL,
>  	    "", NULL, format_headers_message_part_text, "\n",
> @@ -142,6 +149,7 @@ static const notmuch_show_format_t format_raw = {
>              "",
>  	"", "",
>      ""
> +    /* *INDENT-ON* */
>  };
>  
>  static const char *
> diff --git a/notmuch-time.c b/notmuch-time.c
> index e250c3d..a223a99 100644
> --- a/notmuch-time.c
> +++ b/notmuch-time.c
> @@ -38,9 +38,12 @@
>   * (if any) will be reclaimed.
>   *
>   */
> +
>  #define MINUTE (60)
> +/* *INDENT-OFF* -- smells like a bug in uncrustify (up to 0.59...) */
>  #define HOUR (60 * MINUTE)
>  #define DAY (24 * HOUR)
> +/* *INDENT-ON* */
>  #define RELATIVE_DATE_MAX 20
>  const char *
>  notmuch_time_relative_date (const void *ctx, time_t then)
> -- 
> 1.7.8.2
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch