All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: John Szakmeister <john@szakmeister.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] contrib/git-credential-gnome-keyring.c: small stylistic cleanups
Date: Mon, 09 Dec 2013 10:06:15 -0800	[thread overview]
Message-ID: <xmqq38m1292g.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1386066764-49711-1-git-send-email-john@szakmeister.net> (John Szakmeister's message of "Tue, 3 Dec 2013 05:32:44 -0500")

John Szakmeister <john@szakmeister.net> writes:

> Signed-off-by: John Szakmeister <john@szakmeister.net>
> ---
> The gnome-keyring credential backend had a number of coding style
> violations.  I believe this fixes all of them.
>
>  .../gnome-keyring/git-credential-gnome-keyring.c   | 55 ++++++++++------------
>  1 file changed, 25 insertions(+), 30 deletions(-)
>
> diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
> index 635c96b..1613404 100644
> --- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
> +++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
> @@ -95,9 +95,9 @@ static const char* gnome_keyring_result_to_message(GnomeKeyringResult result)
>  
>  static void gnome_keyring_done_cb(GnomeKeyringResult result, gpointer user_data)
>  {
> -	gpointer *data = (gpointer*) user_data;
> -	int *done = (int*) data[0];
> -	GnomeKeyringResult *r = (GnomeKeyringResult*) data[1];
> +	gpointer *data = (gpointer *) user_data;
> +	int *done = (int *) data[0];
> +	GnomeKeyringResult *r = (GnomeKeyringResult *) data[1];

I thought we cast without SP after the (typename), i.e.

	gpointer *data = (gpointer *)user_data;

It could be argued that a cast that turns a "void *" to a pointer to
another type can go, as Felipe noted, but I think that is better
done in a separate patch, perhaps as a follow-up to this "small
stylistic clean-ups".

I said "it could be argued" above, because I am on the fence on that
change.  If this were not using a type "gpointer", whose point is to
hide what the actual implementation of that type is, but a plain
vanilla "void *", then I would not have any doubt.  But it feels
wrong to look behind that deliberate "gpointer" abstraction and take
advantage of the knowledge that it happens to be implemented as
"void *" (and if we do not start from that knowledge, losing the
cast is a wrong change).

So...

  parent reply	other threads:[~2013-12-09 18:06 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-03 10:32 [PATCH] contrib/git-credential-gnome-keyring.c: small stylistic cleanups John Szakmeister
2013-12-05  0:16 ` Junio C Hamano
2013-12-07  9:37 ` Felipe Contreras
2013-12-07 10:42 ` [PATCH v2] " John Szakmeister
2013-12-09 18:06 ` Junio C Hamano [this message]
2013-12-10 11:13   ` [PATCH] " John Szakmeister
2013-12-12 23:41     ` Junio C Hamano
2013-12-13 20:07       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq38m1292g.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=john@szakmeister.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.