Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Miguel Angel Vico]

Thank you all for your clarifications.

I'm about to send updated revisions of all patches. I made a rebase and
fixed all indentation/alignment issues across all patches.

Thanks.

On Wed, 18 May 2016 15:31:32 +0100
Daniel Stone  wrote:

> Hi,
> 
> On 18 May 2016 at 15:25, Derek Foreman  wrote:
> > On 18/05/16 08:41 AM, Mike Blumenkrantz wrote:  
> >> In fairness, we'd likely be less short on review bandwidth if the
> >> majority of that bandwidth was not in use to make/revise trivial
> >> criticisms such as whitespace usage and comment grammar which are
> >> guaranteed to be cleaned up in future patches; this leads to
> >> burnout on both the code-writing side as well as the reviewing
> >> side since everyone has become hyper attuned to the insignificant
> >> and non-functional minutiae of patches rather than focusing more
> >> on expediting the technical development of the protocol.  
> >
> > Fair points, though I'm not certain "will certainly get fixed up
> > later" is a given.  Certainly indenting and basic style is a
> > mechanical problem that could be tested pre-commit hooks, and there
> > should be no reason to bike shed that on the list at all.
> >
> > Grammar probably needs more serious consideration for protocol doc
> > than elsewhere due to its potential impact on compositor
> > implementors - but ever there probably not to the degree we're
> > seeing lately.
> >
> > Follow up commits that do nothing but change style and grammar can
> > make "git blame" less useful (when trying to figure out who would
> > best review a piece of code - not just "agh who wrote this
> > stupid bug") and provide churn for very little benefit, imho.  
> 
> Yeah, I agree. I get that the bikeshedding can be annoying; I do (for
> that reason, if no other) like tagging commits as 'RFC' or similar,
> which is effectively, 'please just check out the technical concept and
> don't worry about memory leaks or spelling mistakes right now'. But
> given that it's pretty trivial to fix up, and you're likely to have to
> rebase _anyway_, I don't see the harm in doing one round of review for
> clarity.
> 
> Generally, there's no need to send out a subsequent revision round
> just because someone has noted some typos - send it again if you need
> a resend anyway to get people to pick it up after a rebase, or if
> there have been notable changes, but you shouldn't be arriving at v17
> just because you have difficulty spelling.
> 
> Similarly, 'no, I disagree' is a reasonable response to someone
> bikeshedding your exact choice of variables or naming. Review is meant
> to be a discussion, not something you just have to unilaterally
> acqiuesce to.
> 
> > While we're drifting just slightly off topic here, I'm also
> > concerned about the basic usage of some of our tags:
> >
> > Reviewed-by:  indicates rigorous technical review *AND* a firm
> > conviction that the feature is important and should be included.
> >
> > Acked-by: Indicates a firm conviction that the feature is important
> > and should be included, but no rigorous review has taken place.
> >
> > Signed-off-by: Indicates an assumption of responsibility for the
> > code.
> >
> > I've seen a lot of Reviewed-by that I think is just "I looked at the
> > spelling and you're good to go".  I'm starting to find this
> > disconcerting.  
> 
> Yes, completely agreed. There's a huge gulf between 'I looked at this
> and nothing bad jumped out', versus 'I understand the implications
> with this and am prepared to put my name down in agreeance with it
> being a good idea'. I am pretty bad with review latency in general,
> but on the other hand I do at least give them a reasonably thorough
> look over ...
> 
> Cheers,
> Daniel


-- 
Miguel


___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Daniel Stone]

Hi,

On 18 May 2016 at 15:25, Derek Foreman  wrote:
> On 18/05/16 08:41 AM, Mike Blumenkrantz wrote:
>> In fairness, we'd likely be less short on review bandwidth if the
>> majority of that bandwidth was not in use to make/revise trivial
>> criticisms such as whitespace usage and comment grammar which are
>> guaranteed to be cleaned up in future patches; this leads to burnout on
>> both the code-writing side as well as the reviewing side since everyone
>> has become hyper attuned to the insignificant and non-functional
>> minutiae of patches rather than focusing more on expediting the
>> technical development of the protocol.
>
> Fair points, though I'm not certain "will certainly get fixed up later"
> is a given.  Certainly indenting and basic style is a mechanical problem
> that could be tested pre-commit hooks, and there should be no reason to
> bike shed that on the list at all.
>
> Grammar probably needs more serious consideration for protocol doc than
> elsewhere due to its potential impact on compositor implementors - but
> ever there probably not to the degree we're seeing lately.
>
> Follow up commits that do nothing but change style and grammar can make
> "git blame" less useful (when trying to figure out who would best review
> a piece of code - not just "agh who wrote this stupid bug") and
> provide churn for very little benefit, imho.

Yeah, I agree. I get that the bikeshedding can be annoying; I do (for
that reason, if no other) like tagging commits as 'RFC' or similar,
which is effectively, 'please just check out the technical concept and
don't worry about memory leaks or spelling mistakes right now'. But
given that it's pretty trivial to fix up, and you're likely to have to
rebase _anyway_, I don't see the harm in doing one round of review for
clarity.

Generally, there's no need to send out a subsequent revision round
just because someone has noted some typos - send it again if you need
a resend anyway to get people to pick it up after a rebase, or if
there have been notable changes, but you shouldn't be arriving at v17
just because you have difficulty spelling.

Similarly, 'no, I disagree' is a reasonable response to someone
bikeshedding your exact choice of variables or naming. Review is meant
to be a discussion, not something you just have to unilaterally
acqiuesce to.

> While we're drifting just slightly off topic here, I'm also concerned
> about the basic usage of some of our tags:
>
> Reviewed-by:  indicates rigorous technical review *AND* a firm
> conviction that the feature is important and should be included.
>
> Acked-by: Indicates a firm conviction that the feature is important and
> should be included, but no rigorous review has taken place.
>
> Signed-off-by: Indicates an assumption of responsibility for the code.
>
> I've seen a lot of Reviewed-by that I think is just "I looked at the
> spelling and you're good to go".  I'm starting to find this disconcerting.

Yes, completely agreed. There's a huge gulf between 'I looked at this
and nothing bad jumped out', versus 'I understand the implications
with this and am prepared to put my name down in agreeance with it
being a good idea'. I am pretty bad with review latency in general,
but on the other hand I do at least give them a reasonably thorough
look over ...

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Derek Foreman]

On 18/05/16 08:41 AM, Mike Blumenkrantz wrote:
> 
> 
> On Wed, May 18, 2016 at 3:51 AM Pekka Paalanen  > wrote:
> 
> On Thu, 12 May 2016 17:20:49 +0200
> Miguel Angel Vico  > wrote:
> 
> > Thanks Derek.
> >
> > Inline.
> >
> > On Thu, 12 May 2016 08:54:26 -0500
> > Derek Foreman  > wrote:
> >
> > > On 11/05/16 10:53 AM, Miguel Angel Vico wrote:
> > > > Thanks, Yong.
> > > >
> > > > Inline.
> > > >
> > > > On Wed, 11 May 2016 09:45:15 -0500
> > > > Yong Bakos  > wrote:
> > > >
> > > >> On May 11, 2016, at 7:48 AM, Miguel A. Vico
> mailto:mvicom...@nvidia.com>>
> > > >> wrote:
> > > >>>
> > > >>> No functional change. This patch only renames
> gl_renderer_create()
> > > >>> to gl_renderer_display_create(), which is something more
> > > >>> descriptive of what the function does.
> > > >>>
> > > >>> Signed-off-by: Miguel A Vico Moya  >
> > > >>> Reviewed-by: James Jones  >
> > > >>
> > > >> Hi Miguel,
> > > >> When renaming, I suggest adjusting the indentation of subsequent
> > > >> arguments as well, to preserve alignment. Example noted inline
> > > >> below. I'm noticing this issue throughout this whole patch
> > > >> series.
> > > >
> > > > The thing is arguments aren't correctly aligned throughout all
> > > > weston code. I think the code-style rule of using hard-tabs
> instead
> > > > of spaces for indentation made things to be a bit messy, since
> > > > arguments are usually aligned using hard-tabs as well, but I think
> > > > spaces should be used instead for those cases in order to keep a
> > > > correct indentation regardless tab length settings of the editor.
> > >
> > > The style of this weston source code is to use hard tabs, 8 chars
> > > wide. It won't look right in an editor configured with any other tab
> > > width.
> > >
> > > I'm with Yong on this one - if you're going to rename a
> function, it's
> > > preferable to fix up the indenting at the same time for those call
> > > sites (even if they didn't match the style guidelines before).  This
> > > fix-up should be done with tabs for alignment.
> > >
> > > Otherwise trivial refactoring patches would rapidly make the
> code look
> > > terrible.
> >
> > Okay, sounds sane to me too. I'll send an updated patch.
> >
> > >
> > > For what it's worth, I personally prefer tabs for indenting and
> spaces
> > > for alignment -
> >
> > I prefer spaces for everything, but I can live with tabs for
> > indenting and spaces for alignment :-)
> >
> > > but that's not what this project uses, so it's not
> > > what I use on this project. :)
> >
> > I double-checked code-style rules here:
> >
> >   https://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing
> >
> > and wrt alignment it only states:
> >
> >   - when breaking lines with functions calls, the parameters are
> aligned
> > with the opening parentheses;
> >
> > To me, that doesn't mean we shouldn't use spaces for alignment.
> 
> Hi,
> 
> as with all styles, there are exceptions.
> 
> When breaking lines and aligning, it often does not happen exactly at a
> hard-tab boundary, so use spaces for the final adjustment.
> 
> However, if any single argument to a function call cannot be reasonably
> fit by the alignment requirement without breaking the max line length,
> then (at least I have) you can forget the alignment to the opening
> parenthesis and just indent as far as you can and align there, using
> hard-tabs only.
> 
> Sometimes it might be preferable to use shortcut variables, sometimes
> not. Rules can be bent if it is necessary for style consistency and
> readability. After all, the whole point of a style definition is to make
> things more readable to everyone on average, and almost always
> consistency in style helps that, so people should follow the project
> style rather than personal preferences.
> 
> And yes, Weston does not rigorously follow the defined style to the
> point absolutely everywhere. Sometimes it is just better to land a v6
> of a patch than to go a few more review cycles to hone the style. Even
> reviewers can get tired of it, and we're usually short on review
> bandwidth anyway.
> 
> 
> In fairness, we'd likely be less short on review bandwidth if the
> majority of that bandwidth was not in use to make/revise trivial
> criticisms such as whitespace usage and comment grammar which are
> guaranteed to be cleaned up in future patches; this leads to burnout on
> both the code-writi

Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Mike Blumenkrantz]

On Wed, May 18, 2016 at 3:51 AM Pekka Paalanen  wrote:

> On Thu, 12 May 2016 17:20:49 +0200
> Miguel Angel Vico  wrote:
>
> > Thanks Derek.
> >
> > Inline.
> >
> > On Thu, 12 May 2016 08:54:26 -0500
> > Derek Foreman  wrote:
> >
> > > On 11/05/16 10:53 AM, Miguel Angel Vico wrote:
> > > > Thanks, Yong.
> > > >
> > > > Inline.
> > > >
> > > > On Wed, 11 May 2016 09:45:15 -0500
> > > > Yong Bakos  wrote:
> > > >
> > > >> On May 11, 2016, at 7:48 AM, Miguel A. Vico 
> > > >> wrote:
> > > >>>
> > > >>> No functional change. This patch only renames gl_renderer_create()
> > > >>> to gl_renderer_display_create(), which is something more
> > > >>> descriptive of what the function does.
> > > >>>
> > > >>> Signed-off-by: Miguel A Vico Moya 
> > > >>> Reviewed-by: James Jones 
> > > >>
> > > >> Hi Miguel,
> > > >> When renaming, I suggest adjusting the indentation of subsequent
> > > >> arguments as well, to preserve alignment. Example noted inline
> > > >> below. I'm noticing this issue throughout this whole patch
> > > >> series.
> > > >
> > > > The thing is arguments aren't correctly aligned throughout all
> > > > weston code. I think the code-style rule of using hard-tabs instead
> > > > of spaces for indentation made things to be a bit messy, since
> > > > arguments are usually aligned using hard-tabs as well, but I think
> > > > spaces should be used instead for those cases in order to keep a
> > > > correct indentation regardless tab length settings of the editor.
> > >
> > > The style of this weston source code is to use hard tabs, 8 chars
> > > wide. It won't look right in an editor configured with any other tab
> > > width.
> > >
> > > I'm with Yong on this one - if you're going to rename a function, it's
> > > preferable to fix up the indenting at the same time for those call
> > > sites (even if they didn't match the style guidelines before).  This
> > > fix-up should be done with tabs for alignment.
> > >
> > > Otherwise trivial refactoring patches would rapidly make the code look
> > > terrible.
> >
> > Okay, sounds sane to me too. I'll send an updated patch.
> >
> > >
> > > For what it's worth, I personally prefer tabs for indenting and spaces
> > > for alignment -
> >
> > I prefer spaces for everything, but I can live with tabs for
> > indenting and spaces for alignment :-)
> >
> > > but that's not what this project uses, so it's not
> > > what I use on this project. :)
> >
> > I double-checked code-style rules here:
> >
> >   https://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing
> >
> > and wrt alignment it only states:
> >
> >   - when breaking lines with functions calls, the parameters are aligned
> > with the opening parentheses;
> >
> > To me, that doesn't mean we shouldn't use spaces for alignment.
>
> Hi,
>
> as with all styles, there are exceptions.
>
> When breaking lines and aligning, it often does not happen exactly at a
> hard-tab boundary, so use spaces for the final adjustment.
>
> However, if any single argument to a function call cannot be reasonably
> fit by the alignment requirement without breaking the max line length,
> then (at least I have) you can forget the alignment to the opening
> parenthesis and just indent as far as you can and align there, using
> hard-tabs only.
>
> Sometimes it might be preferable to use shortcut variables, sometimes
> not. Rules can be bent if it is necessary for style consistency and
> readability. After all, the whole point of a style definition is to make
> things more readable to everyone on average, and almost always
> consistency in style helps that, so people should follow the project
> style rather than personal preferences.
>
> And yes, Weston does not rigorously follow the defined style to the
> point absolutely everywhere. Sometimes it is just better to land a v6
> of a patch than to go a few more review cycles to hone the style. Even
> reviewers can get tired of it, and we're usually short on review
> bandwidth anyway.
>

In fairness, we'd likely be less short on review bandwidth if the majority
of that bandwidth was not in use to make/revise trivial criticisms such as
whitespace usage and comment grammar which are guaranteed to be cleaned up
in future patches; this leads to burnout on both the code-writing side as
well as the reviewing side since everyone has become hyper attuned to the
insignificant and non-functional minutiae of patches rather than focusing
more on expediting the technical development of the protocol.


>
>
> Thanks,
> pq
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Pekka Paalanen]

On Thu, 12 May 2016 17:20:49 +0200
Miguel Angel Vico  wrote:

> Thanks Derek.
> 
> Inline.
> 
> On Thu, 12 May 2016 08:54:26 -0500
> Derek Foreman  wrote:
> 
> > On 11/05/16 10:53 AM, Miguel Angel Vico wrote:  
> > > Thanks, Yong.
> > > 
> > > Inline.
> > > 
> > > On Wed, 11 May 2016 09:45:15 -0500
> > > Yong Bakos  wrote:
> > > 
> > >> On May 11, 2016, at 7:48 AM, Miguel A. Vico 
> > >> wrote:
> > >>>
> > >>> No functional change. This patch only renames gl_renderer_create()
> > >>> to gl_renderer_display_create(), which is something more
> > >>> descriptive of what the function does.
> > >>>
> > >>> Signed-off-by: Miguel A Vico Moya 
> > >>> Reviewed-by: James Jones   
> > >>
> > >> Hi Miguel,
> > >> When renaming, I suggest adjusting the indentation of subsequent
> > >> arguments as well, to preserve alignment. Example noted inline
> > >> below. I'm noticing this issue throughout this whole patch
> > >> series.
> > > 
> > > The thing is arguments aren't correctly aligned throughout all
> > > weston code. I think the code-style rule of using hard-tabs instead
> > > of spaces for indentation made things to be a bit messy, since
> > > arguments are usually aligned using hard-tabs as well, but I think
> > > spaces should be used instead for those cases in order to keep a
> > > correct indentation regardless tab length settings of the editor.
> > 
> > The style of this weston source code is to use hard tabs, 8 chars
> > wide. It won't look right in an editor configured with any other tab
> > width.
> > 
> > I'm with Yong on this one - if you're going to rename a function, it's
> > preferable to fix up the indenting at the same time for those call
> > sites (even if they didn't match the style guidelines before).  This
> > fix-up should be done with tabs for alignment.
> > 
> > Otherwise trivial refactoring patches would rapidly make the code look
> > terrible.  
> 
> Okay, sounds sane to me too. I'll send an updated patch.
> 
> > 
> > For what it's worth, I personally prefer tabs for indenting and spaces
> > for alignment -   
> 
> I prefer spaces for everything, but I can live with tabs for
> indenting and spaces for alignment :-)
> 
> > but that's not what this project uses, so it's not
> > what I use on this project. :)  
> 
> I double-checked code-style rules here:
> 
>   https://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing
> 
> and wrt alignment it only states:
> 
>   - when breaking lines with functions calls, the parameters are aligned
> with the opening parentheses;
> 
> To me, that doesn't mean we shouldn't use spaces for alignment.

Hi,

as with all styles, there are exceptions.

When breaking lines and aligning, it often does not happen exactly at a
hard-tab boundary, so use spaces for the final adjustment.

However, if any single argument to a function call cannot be reasonably
fit by the alignment requirement without breaking the max line length,
then (at least I have) you can forget the alignment to the opening
parenthesis and just indent as far as you can and align there, using
hard-tabs only.

Sometimes it might be preferable to use shortcut variables, sometimes
not. Rules can be bent if it is necessary for style consistency and
readability. After all, the whole point of a style definition is to make
things more readable to everyone on average, and almost always
consistency in style helps that, so people should follow the project
style rather than personal preferences.

And yes, Weston does not rigorously follow the defined style to the
point absolutely everywhere. Sometimes it is just better to land a v6
of a patch than to go a few more review cycles to hone the style. Even
reviewers can get tired of it, and we're usually short on review
bandwidth anyway.


Thanks,
pq


pgpFMac_wXfPE.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Miguel Angel Vico]

Thanks Derek.

Inline.

On Thu, 12 May 2016 08:54:26 -0500
Derek Foreman  wrote:

> On 11/05/16 10:53 AM, Miguel Angel Vico wrote:
> > Thanks, Yong.
> > 
> > Inline.
> > 
> > On Wed, 11 May 2016 09:45:15 -0500
> > Yong Bakos  wrote:
> >   
> >> On May 11, 2016, at 7:48 AM, Miguel A. Vico 
> >> wrote:  
> >>>
> >>> No functional change. This patch only renames gl_renderer_create()
> >>> to gl_renderer_display_create(), which is something more
> >>> descriptive of what the function does.
> >>>
> >>> Signed-off-by: Miguel A Vico Moya 
> >>> Reviewed-by: James Jones 
> >>
> >> Hi Miguel,
> >> When renaming, I suggest adjusting the indentation of subsequent
> >> arguments as well, to preserve alignment. Example noted inline
> >> below. I'm noticing this issue throughout this whole patch
> >> series.  
> > 
> > The thing is arguments aren't correctly aligned throughout all
> > weston code. I think the code-style rule of using hard-tabs instead
> > of spaces for indentation made things to be a bit messy, since
> > arguments are usually aligned using hard-tabs as well, but I think
> > spaces should be used instead for those cases in order to keep a
> > correct indentation regardless tab length settings of the editor.  
> 
> The style of this weston source code is to use hard tabs, 8 chars
> wide. It won't look right in an editor configured with any other tab
> width.
> 
> I'm with Yong on this one - if you're going to rename a function, it's
> preferable to fix up the indenting at the same time for those call
> sites (even if they didn't match the style guidelines before).  This
> fix-up should be done with tabs for alignment.
> 
> Otherwise trivial refactoring patches would rapidly make the code look
> terrible.

Okay, sounds sane to me too. I'll send an updated patch.

> 
> For what it's worth, I personally prefer tabs for indenting and spaces
> for alignment - 

I prefer spaces for everything, but I can live with tabs for
indenting and spaces for alignment :-)

> but that's not what this project uses, so it's not
> what I use on this project. :)

I double-checked code-style rules here:

  https://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing

and wrt alignment it only states:

  - when breaking lines with functions calls, the parameters are aligned
with the opening parentheses;

To me, that doesn't mean we shouldn't use spaces for alignment.

> 
> > I wanted to be as conservative as possible with my patches, and
> > don't add many of those tab-by-space-replacement changes.
> > 
> > I can update my patches, if that's fine, or prepare a separate set
> > of changes to only fix those indentation issues.
> > 
> > Please, let me know what's preferred.  
> 
> I general, fixing up the patches would be best, and we appreciate the
> extra work - however, I'm not sure about this particular patch.
> 
> The function sets up a bunch of function pointers, and does more than
> just create a display.  It also has a symmetrical
> gl_renderer_destroy() which hasn't been renamed.
> 
> I don't think adding the word "display" here really makes anything
> more clear.

The main purpose of that function is to get the corresponding
EGLDisplay connection and bind both EGLDisplay and wl_display. I think
I'm with Daniel Stone on this and
s/gl_renderer_create/gl_renderer_display_create/ or
s/gl_renderer_create/gl_renderer_bind_display/ is a bit more
descriptive.

I agree with you we should rename gl_renderer_destroy() as well.


I'll send updated versions of these patches.

Thanks.

> 
> Thanks,
> Derek
> 
> > Thanks,
> > Miguel.
> >   
> >>
> >> Respectfully,
> >> yong
> >>
> >>  
> >>> ---
> >>> src/compositor-drm.c | 2 +-
> >>> src/compositor-fbdev.c   | 2 +-
> >>> src/compositor-wayland.c | 2 +-
> >>> src/compositor-x11.c | 5 +++--
> >>> src/gl-renderer.c| 4 ++--
> >>> src/gl-renderer.h| 2 +-
> >>> 6 files changed, 9 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> >>> index a47b453..9c58710 100644
> >>> --- a/src/compositor-drm.c
> >>> +++ b/src/compositor-drm.c
> >>> @@ -1585,7 +1585,7 @@ drm_backend_create_gl_renderer(struct
> >>> drm_backend *b)
> >>>
> >>>   if (format[1])
> >>>   n_formats = 3;
> >>> - if (gl_renderer->create(b->compositor,
> >>> + if (gl_renderer->display_create(b->compositor,
> >>>   EGL_PLATFORM_GBM_KHR,
> >>>   (void *)b->gbm,
> >>>   gl_renderer->opaque_attribs,
> >>
> >> The subsequent args should be aligned with the first (the 'b' in
> >> 'b->compositor').
> >>
> >>
> >>  
> >>> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> >>> index ee762e3..c6ffcd7 100644
> >>> --- a/src/compositor-fbdev.c
> >>> +++ b/src/compositor-fbdev.c
> >>> @@ -785,7 +785,7 @@ fbdev_backend_create(struct weston_compositor
> >>> *compositor, int *argc, char *argv goto out_launcher;
> >>>   }
> >>>
> >>> - if (gl_renderer->c

Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Derek Foreman]

On 11/05/16 10:53 AM, Miguel Angel Vico wrote:
> Thanks, Yong.
> 
> Inline.
> 
> On Wed, 11 May 2016 09:45:15 -0500
> Yong Bakos  wrote:
> 
>> On May 11, 2016, at 7:48 AM, Miguel A. Vico 
>> wrote:
>>>
>>> No functional change. This patch only renames gl_renderer_create()
>>> to gl_renderer_display_create(), which is something more
>>> descriptive of what the function does.
>>>
>>> Signed-off-by: Miguel A Vico Moya 
>>> Reviewed-by: James Jones   
>>
>> Hi Miguel,
>> When renaming, I suggest adjusting the indentation of subsequent
>> arguments as well, to preserve alignment. Example noted inline below.
>> I'm noticing this issue throughout this whole patch series.
> 
> The thing is arguments aren't correctly aligned throughout all weston
> code. I think the code-style rule of using hard-tabs instead of spaces
> for indentation made things to be a bit messy, since arguments are
> usually aligned using hard-tabs as well, but I think spaces should be
> used instead for those cases in order to keep a correct indentation
> regardless tab length settings of the editor.

The style of this weston source code is to use hard tabs, 8 chars wide.
 It won't look right in an editor configured with any other tab width.

I'm with Yong on this one - if you're going to rename a function, it's
preferable to fix up the indenting at the same time for those call sites
(even if they didn't match the style guidelines before).  This fix-up
should be done with tabs for alignment.

Otherwise trivial refactoring patches would rapidly make the code look
terrible.

For what it's worth, I personally prefer tabs for indenting and spaces
for alignment - but that's not what this project uses, so it's not what
I use on this project. :)

> I wanted to be as conservative as possible with my patches, and don't
> add many of those tab-by-space-replacement changes.
> 
> I can update my patches, if that's fine, or prepare a separate set
> of changes to only fix those indentation issues.
> 
> Please, let me know what's preferred.

I general, fixing up the patches would be best, and we appreciate the
extra work - however, I'm not sure about this particular patch.

The function sets up a bunch of function pointers, and does more than
just create a display.  It also has a symmetrical gl_renderer_destroy()
which hasn't been renamed.

I don't think adding the word "display" here really makes anything more
clear.

Thanks,
Derek

> Thanks,
> Miguel.
> 
>>
>> Respectfully,
>> yong
>>
>>
>>> ---
>>> src/compositor-drm.c | 2 +-
>>> src/compositor-fbdev.c   | 2 +-
>>> src/compositor-wayland.c | 2 +-
>>> src/compositor-x11.c | 5 +++--
>>> src/gl-renderer.c| 4 ++--
>>> src/gl-renderer.h| 2 +-
>>> 6 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
>>> index a47b453..9c58710 100644
>>> --- a/src/compositor-drm.c
>>> +++ b/src/compositor-drm.c
>>> @@ -1585,7 +1585,7 @@ drm_backend_create_gl_renderer(struct
>>> drm_backend *b)
>>>
>>> if (format[1])
>>> n_formats = 3;
>>> -   if (gl_renderer->create(b->compositor,
>>> +   if (gl_renderer->display_create(b->compositor,
>>> EGL_PLATFORM_GBM_KHR,
>>> (void *)b->gbm,
>>> gl_renderer->opaque_attribs,  
>>
>> The subsequent args should be aligned with the first (the 'b' in
>> 'b->compositor').
>>
>>
>>
>>> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
>>> index ee762e3..c6ffcd7 100644
>>> --- a/src/compositor-fbdev.c
>>> +++ b/src/compositor-fbdev.c
>>> @@ -785,7 +785,7 @@ fbdev_backend_create(struct weston_compositor
>>> *compositor, int *argc, char *argv goto out_launcher;
>>> }
>>>
>>> -   if (gl_renderer->create(compositor,
>>> NO_EGL_PLATFORM,
>>> +   if (gl_renderer->display_create(compositor,
>>> NO_EGL_PLATFORM, EGL_DEFAULT_DISPLAY,
>>> gl_renderer->opaque_attribs,
>>> NULL, 0) < 0) {
>>> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
>>> index d0d1082..e4595c7 100644
>>> --- a/src/compositor-wayland.c
>>> +++ b/src/compositor-wayland.c
>>> @@ -2261,7 +2261,7 @@ wayland_backend_create(struct
>>> weston_compositor *compositor, }
>>>
>>> if (!b->use_pixman) {
>>> -   if (gl_renderer->create(compositor,
>>> +   if (gl_renderer->display_create(compositor,
>>> EGL_PLATFORM_WAYLAND_KHR,
>>> b->parent.wl_display,
>>> gl_renderer->alpha_attribs,
>>> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
>>> index 629b5f3..78b9c62 100644
>>> --- a/src/compositor-x11.c
>>> +++ b/src/compositor-x11.c
>>> @@ -1557,8 +1557,9 @@ init_gl_renderer(struct x11_backend *b)
>>> if (!gl_renderer)
>>> return -1;
>>>
>>> -   ret = gl_renderer->create(b->compositor,
>>> EGL_PLAT

Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Miguel Angel Vico]

Thanks, Yong.

Inline.

On Wed, 11 May 2016 09:45:15 -0500
Yong Bakos  wrote:

> On May 11, 2016, at 7:48 AM, Miguel A. Vico 
> wrote:
> > 
> > No functional change. This patch only renames gl_renderer_create()
> > to gl_renderer_display_create(), which is something more
> > descriptive of what the function does.
> > 
> > Signed-off-by: Miguel A Vico Moya 
> > Reviewed-by: James Jones   
> 
> Hi Miguel,
> When renaming, I suggest adjusting the indentation of subsequent
> arguments as well, to preserve alignment. Example noted inline below.
> I'm noticing this issue throughout this whole patch series.

The thing is arguments aren't correctly aligned throughout all weston
code. I think the code-style rule of using hard-tabs instead of spaces
for indentation made things to be a bit messy, since arguments are
usually aligned using hard-tabs as well, but I think spaces should be
used instead for those cases in order to keep a correct indentation
regardless tab length settings of the editor.

I wanted to be as conservative as possible with my patches, and don't
add many of those tab-by-space-replacement changes.

I can update my patches, if that's fine, or prepare a separate set
of changes to only fix those indentation issues.

Please, let me know what's preferred.

Thanks,
Miguel.

> 
> Respectfully,
> yong
> 
> 
> > ---
> > src/compositor-drm.c | 2 +-
> > src/compositor-fbdev.c   | 2 +-
> > src/compositor-wayland.c | 2 +-
> > src/compositor-x11.c | 5 +++--
> > src/gl-renderer.c| 4 ++--
> > src/gl-renderer.h| 2 +-
> > 6 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > index a47b453..9c58710 100644
> > --- a/src/compositor-drm.c
> > +++ b/src/compositor-drm.c
> > @@ -1585,7 +1585,7 @@ drm_backend_create_gl_renderer(struct
> > drm_backend *b)
> > 
> > if (format[1])
> > n_formats = 3;
> > -   if (gl_renderer->create(b->compositor,
> > +   if (gl_renderer->display_create(b->compositor,
> > EGL_PLATFORM_GBM_KHR,
> > (void *)b->gbm,
> > gl_renderer->opaque_attribs,  
> 
> The subsequent args should be aligned with the first (the 'b' in
> 'b->compositor').
> 
> 
> 
> > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> > index ee762e3..c6ffcd7 100644
> > --- a/src/compositor-fbdev.c
> > +++ b/src/compositor-fbdev.c
> > @@ -785,7 +785,7 @@ fbdev_backend_create(struct weston_compositor
> > *compositor, int *argc, char *argv goto out_launcher;
> > }
> > 
> > -   if (gl_renderer->create(compositor,
> > NO_EGL_PLATFORM,
> > +   if (gl_renderer->display_create(compositor,
> > NO_EGL_PLATFORM, EGL_DEFAULT_DISPLAY,
> > gl_renderer->opaque_attribs,
> > NULL, 0) < 0) {
> > diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> > index d0d1082..e4595c7 100644
> > --- a/src/compositor-wayland.c
> > +++ b/src/compositor-wayland.c
> > @@ -2261,7 +2261,7 @@ wayland_backend_create(struct
> > weston_compositor *compositor, }
> > 
> > if (!b->use_pixman) {
> > -   if (gl_renderer->create(compositor,
> > +   if (gl_renderer->display_create(compositor,
> > EGL_PLATFORM_WAYLAND_KHR,
> > b->parent.wl_display,
> > gl_renderer->alpha_attribs,
> > diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> > index 629b5f3..78b9c62 100644
> > --- a/src/compositor-x11.c
> > +++ b/src/compositor-x11.c
> > @@ -1557,8 +1557,9 @@ init_gl_renderer(struct x11_backend *b)
> > if (!gl_renderer)
> > return -1;
> > 
> > -   ret = gl_renderer->create(b->compositor,
> > EGL_PLATFORM_X11_KHR, (void *) b->dpy,
> > - gl_renderer->opaque_attribs,
> > NULL, 0);
> > +   ret = gl_renderer->display_create(b->compositor,
> > EGL_PLATFORM_X11_KHR,
> > + (void *) b->dpy,
> > +
> > gl_renderer->opaque_attribs, NULL, 0);
> > 
> > return ret;
> > }
> > diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> > index 23c0cd7..197e5c2 100644
> > --- a/src/gl-renderer.c
> > +++ b/src/gl-renderer.c
> > @@ -2873,7 +2873,7 @@ platform_to_extension(EGLenum platform)
> > }
> > 
> > static int
> > -gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
> > +gl_renderer_display_create(struct weston_compositor *ec, EGLenum
> > platform, void *native_window, const EGLint *attribs,
> > const EGLint *visual_id, int n_ids)
> > {
> > @@ -3147,7 +3147,7 @@ WL_EXPORT struct gl_renderer_interface
> > gl_renderer_interface = { .opaque_attribs =
> > gl_renderer_opaque_attribs, .alpha_attribs =
> > gl_renderer_alpha_attribs,
> > 
> > -   .create = gl_renderer_create,
> > +   .display_create = gl_renderer_display_create,
> > .display = gl_renderer_display,
> 

Re: [PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Yong Bakos]

On May 11, 2016, at 7:48 AM, Miguel A. Vico  wrote:
> 
> No functional change. This patch only renames gl_renderer_create() to
> gl_renderer_display_create(), which is something more descriptive of
> what the function does.
> 
> Signed-off-by: Miguel A Vico Moya 
> Reviewed-by: James Jones 

Hi Miguel,
When renaming, I suggest adjusting the indentation of subsequent arguments
as well, to preserve alignment. Example noted inline below. I'm noticing
this issue throughout this whole patch series.

Respectfully,
yong


> ---
> src/compositor-drm.c | 2 +-
> src/compositor-fbdev.c   | 2 +-
> src/compositor-wayland.c | 2 +-
> src/compositor-x11.c | 5 +++--
> src/gl-renderer.c| 4 ++--
> src/gl-renderer.h| 2 +-
> 6 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index a47b453..9c58710 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -1585,7 +1585,7 @@ drm_backend_create_gl_renderer(struct drm_backend *b)
> 
>   if (format[1])
>   n_formats = 3;
> - if (gl_renderer->create(b->compositor,
> + if (gl_renderer->display_create(b->compositor,
>   EGL_PLATFORM_GBM_KHR,
>   (void *)b->gbm,
>   gl_renderer->opaque_attribs,

The subsequent args should be aligned with the first (the 'b' in 
'b->compositor').



> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> index ee762e3..c6ffcd7 100644
> --- a/src/compositor-fbdev.c
> +++ b/src/compositor-fbdev.c
> @@ -785,7 +785,7 @@ fbdev_backend_create(struct weston_compositor 
> *compositor, int *argc, char *argv
>   goto out_launcher;
>   }
> 
> - if (gl_renderer->create(compositor, NO_EGL_PLATFORM,
> + if (gl_renderer->display_create(compositor, NO_EGL_PLATFORM,
>   EGL_DEFAULT_DISPLAY,
>   gl_renderer->opaque_attribs,
>   NULL, 0) < 0) {
> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index d0d1082..e4595c7 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c
> @@ -2261,7 +2261,7 @@ wayland_backend_create(struct weston_compositor 
> *compositor,
>   }
> 
>   if (!b->use_pixman) {
> - if (gl_renderer->create(compositor,
> + if (gl_renderer->display_create(compositor,
>   EGL_PLATFORM_WAYLAND_KHR,
>   b->parent.wl_display,
>   gl_renderer->alpha_attribs,
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index 629b5f3..78b9c62 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -1557,8 +1557,9 @@ init_gl_renderer(struct x11_backend *b)
>   if (!gl_renderer)
>   return -1;
> 
> - ret = gl_renderer->create(b->compositor, EGL_PLATFORM_X11_KHR, (void *) 
> b->dpy,
> -   gl_renderer->opaque_attribs, NULL, 0);
> + ret = gl_renderer->display_create(b->compositor, EGL_PLATFORM_X11_KHR,
> +   (void *) b->dpy,
> +   gl_renderer->opaque_attribs, NULL, 0);
> 
>   return ret;
> }
> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index 23c0cd7..197e5c2 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -2873,7 +2873,7 @@ platform_to_extension(EGLenum platform)
> }
> 
> static int
> -gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
> +gl_renderer_display_create(struct weston_compositor *ec, EGLenum platform,
>   void *native_window, const EGLint *attribs,
>   const EGLint *visual_id, int n_ids)
> {
> @@ -3147,7 +3147,7 @@ WL_EXPORT struct gl_renderer_interface 
> gl_renderer_interface = {
>   .opaque_attribs = gl_renderer_opaque_attribs,
>   .alpha_attribs = gl_renderer_alpha_attribs,
> 
> - .create = gl_renderer_create,
> + .display_create = gl_renderer_display_create,
>   .display = gl_renderer_display,
>   .output_create = gl_renderer_output_create,
>   .output_destroy = gl_renderer_output_destroy,
> diff --git a/src/gl-renderer.h b/src/gl-renderer.h
> index 71f6b46..cd67a89 100644
> --- a/src/gl-renderer.h
> +++ b/src/gl-renderer.h
> @@ -75,7 +75,7 @@ struct gl_renderer_interface {
>   const EGLint *opaque_attribs;
>   const EGLint *alpha_attribs;
> 
> - int (*create)(struct weston_compositor *ec,
> + int (*display_create)(struct weston_compositor *ec,
> EGLenum platform,
> void *native_window,
> const EGLint *attribs,
> -- 
> 2.8.0
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

__