public inbox for git@vger.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...

next prev 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$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=john@szakmeister$(echo .)net \
    /path/to/YOUR_REPLY

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox