Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
Hi, On Wed, Jun 14, 2017 at 04:24:17PM +, Marc-André Lureau wrote: > On Thu, Jun 8, 2017 at 12:59 PM Christophe Fergeau > 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.lur...@redhat.com > > > > > > wrote: > > > > > > > From: Marc-André Lureau > > > > > > > > > > > > > > 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, signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
On Wed, Jun 14, 2017 at 04:24:17PM +, Marc-André Lureau wrote: > On Thu, Jun 8, 2017 at 12:59 PM Christophe Fergeau > wrote: > > 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. 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. "send a patch to change it", I'm not sure what you mean by "it"? Regardless of that, given the back and forth, I'd say you have a strong preference for this, so just keep that patch in the series :) Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
On Thu, Jun 8, 2017 at 12:59 PM Christophe Fergeau 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.lur...@redhat.com > > > > > wrote: > > > > > > From: Marc-André Lureau > > > > > > > > > > > > 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. 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 -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
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.lur...@redhat.com > > > > wrote: > > > > > From: Marc-André Lureau > > > > > > > > > > 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?) Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
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.lur...@redhat.com > > > wrote: > > > > From: Marc-André Lureau > > > > > > > > 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! 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. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
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.lur...@redhat.com wrote: > > > From: Marc-André Lureau > > > > > > 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 :) Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
Hi
- Original Message -
> On Thu, Jun 08, 2017 at 12:55:38AM +0400, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > 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.
Imho private members access are fine with a one letter pointer variable, if
consistently used that way.
> Christophe
>
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > src/spice-widget.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index 1a1d5a6..d948c6d 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -617,10 +617,12 @@ drawing_area_realize(GtkWidget *area, gpointer
> > user_data)
> > {
> > #ifdef GDK_WINDOWING_X11
> > SpiceDisplay *display = SPICE_DISPLAY(user_data);
> > +SpiceDisplayPrivate *d = display->priv;
> >
> > if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > -
> > spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(display->priv->display))
> > != NULL)
> > +spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(d->display))) {
> > spice_display_widget_gl_scanout(display);
> > +}
> >
> > #endif
> > }
> > --
> > 2.13.0.91.g00982b8dd
> >
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
On Thu, Jun 08, 2017 at 12:55:38AM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> 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.
Christophe
>
> Signed-off-by: Marc-André Lureau
> ---
> src/spice-widget.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 1a1d5a6..d948c6d 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -617,10 +617,12 @@ drawing_area_realize(GtkWidget *area, gpointer
> user_data)
> {
> #ifdef GDK_WINDOWING_X11
> SpiceDisplay *display = SPICE_DISPLAY(user_data);
> +SpiceDisplayPrivate *d = display->priv;
>
> if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> -
> spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(display->priv->display))
> != NULL)
> +spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(d->display))) {
> spice_display_widget_gl_scanout(display);
> +}
>
> #endif
> }
> --
> 2.13.0.91.g00982b8dd
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
On Thu, 2017-06-08 at 03:54 -0400, Marc-André Lureau wrote:
> Hi
>
> - Original Message -
> > On Thu, 2017-06-08 at 00:55 +0400, marcandre.lur...@redhat.com
> > wrote:
> > > From: Marc-André Lureau
> > >
> > > Use shorter line, use the common "d" variable for private data
> > > access,
> >
> > just to make the line shorter?
> >
> > > add brackets to ease reading the inner block vs the condition,
> > > remove
> > > needless != NULL.
> >
> > we keep the comparison explicit when it is not boolean
>
> ok,
> ack with that change?
yes, ack with keeping != NULL
Pavel
>
> >
> > Pavel
> > > spice_display_get_gl_scanout
> > > Signed-off-by: Marc-André Lureau
> > > ---
> > > src/spice-widget.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > > index 1a1d5a6..d948c6d 100644
> > > --- a/src/spice-widget.c
> > > +++ b/src/spice-widget.c
> > > @@ -617,10 +617,12 @@ drawing_area_realize(GtkWidget *area,
> > > gpointer
> > > user_data)
> > > {
> > > #ifdef GDK_WINDOWING_X11
> > > SpiceDisplay *display = SPICE_DISPLAY(user_data);
> > > +SpiceDisplayPrivate *d = display->priv;
> > >
> > > if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > > -spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(disp
> > > lay-
> > > > priv->display)) != NULL)
> > >
> > > +spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(d-
> > > > display))) {
> > >
> > > spice_display_widget_gl_scanout(display);
> > > +}
> > >
> > > #endif
> > > }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
Hi
- Original Message -
> On Thu, 2017-06-08 at 00:55 +0400, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > Use shorter line, use the common "d" variable for private data
> > access,
> just to make the line shorter?
>
> > add brackets to ease reading the inner block vs the condition,
> > remove
> > needless != NULL.
>
> we keep the comparison explicit when it is not boolean
ok,
ack with that change?
>
> Pavel
> > spice_display_get_gl_scanout
> > Signed-off-by: Marc-André Lureau
> > ---
> > src/spice-widget.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index 1a1d5a6..d948c6d 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -617,10 +617,12 @@ drawing_area_realize(GtkWidget *area, gpointer
> > user_data)
> > {
> > #ifdef GDK_WINDOWING_X11
> > SpiceDisplay *display = SPICE_DISPLAY(user_data);
> > +SpiceDisplayPrivate *d = display->priv;
> >
> > if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > -spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(display-
> > >priv->display)) != NULL)
> > +spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(d-
> > >display))) {
> > spice_display_widget_gl_scanout(display);
> > +}
> >
> > #endif
> > }
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/5] widget: minor code style improvements
On Thu, 2017-06-08 at 00:55 +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> Use shorter line, use the common "d" variable for private data
> access,
just to make the line shorter?
> add brackets to ease reading the inner block vs the condition,
> remove
> needless != NULL.
we keep the comparison explicit when it is not boolean
Pavel
> spice_display_get_gl_scanout
> Signed-off-by: Marc-André Lureau
> ---
> src/spice-widget.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 1a1d5a6..d948c6d 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -617,10 +617,12 @@ drawing_area_realize(GtkWidget *area, gpointer
> user_data)
> {
> #ifdef GDK_WINDOWING_X11
> SpiceDisplay *display = SPICE_DISPLAY(user_data);
> +SpiceDisplayPrivate *d = display->priv;
>
> if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> -spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(display-
> >priv->display)) != NULL)
> +spice_display_get_gl_scanout(SPICE_DISPLAY_CHANNEL(d-
> >display))) {
> spice_display_widget_gl_scanout(display);
> +}
>
> #endif
> }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
