[Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements

[Victor Toso]

Hi,

On Wed, Jun 14, 2017 at 04:24:17PM +0000, Marc-André Lureau wrote:
> On Thu, Jun 8, 2017 at 12:59 PM Christophe Fergeau <cfergeau at redhat.com>
> wrote:
> 
> > On Thu, Jun 08, 2017 at 04:41:55AM -0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > ----- Original Message -----
> > > > On Thu, Jun 08, 2017 at 04:31:20AM -0400, Marc-André Lureau wrote:
> > > > > Hi
> > > > >
> > > > > ----- Original Message -----
> > > > > > On Thu, Jun 08, 2017 at 12:55:38AM +0400,
> > marcandre.lureau at redhat.com
> > > > > > wrote:
> > > > > > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > > > >
> > > > > > > Use shorter line, use the common "d" variable for private data
> > access,
> > > > > > > add brackets to ease reading the inner block vs the condition,
> > remove
> > > > > > > needless != NULL.
> > > > > >
> > > > > > I'd lean towards NACK for this one, one letter variable names is
> > imo
> > > > > > very bad for readability. I know this is widespread in the
> > spice-gtk
> > > > > > codebase, but I'd at least rather not expand that usage.
> > > > > >
> > > > >
> > > > > You may rename it "priv", but then you lost the benefit of being
> > really
> > > > > short.
> > > >
> > > > Being really short is a benefit? This is where we are going to
> > disagree :)
> > >
> > > Well, in an proper OO language, you wouldn't even have it, it would be
> > like magic!
> >
> > Maybe, maybe not, 'priv' members in C++ classes are not that unusual
> > ( https://en.wikipedia.org/wiki/Opaque_pointer#C.2B.2B )
> >
> > > So yes, I like private member being accessed with a very short
> > > variable in C. If it's use consistently, there is very little
> > > confusion possible imho.
> >
> > As I said, we are not going to agree there :) "If it's used
> > consistently, it does not cause confusion", does not mean that's a good
> > thing :) This just makes things harder to read for someone not knowing
> > the convention, and for no great reason (saving at most a few seconds of
> > typing?)
> >
>
> Ok you don't like that single letter variable, but as maintainer I prefer
> consistency, even though I don't have written rules, I am not strict
> either.

Maybe we should be more strict in order to spend less time discussing
code style.

> If you feel strongly about it, I can drop the patch. I suggest you
> send a patch to change it if you think it will improve readability
> anyway.
>
> thanks

I also don't like one letter variables but you have +1 here to keep
consistency as this is the only access to display->priv that is not done
by 'd'.

My vote would be for 'priv' but in the whole code base, that would be a
clear variable name for *private* structure (and as another great french
hacker said to me once, we are not paying for characters - 'priv' should
be short enough...)

I'm reviewing your series, I'll reply back soon about it.

Cheers,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170615/19e2795c/attachment.sig>