[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 <ppaalanen at gmail.com
> <mailto:ppaalanen at gmail.com>> wrote:
> 
>     On Thu, 12 May 2016 17:20:49 +0200
>     Miguel Angel Vico <mvicomoya at nvidia.com
>     <mailto:mvicomoya at nvidia.com>> wrote:
> 
>     > Thanks Derek.
>     >
>     > Inline.
>     >
>     > On Thu, 12 May 2016 08:54:26 -0500
>     > Derek Foreman <derekf at osg.samsung.com
>     <mailto:derekf at osg.samsung.com>> 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 <junk at humanoriented.com
>     <mailto:junk at humanoriented.com>> wrote:
>     > > >
>     > > >> On May 11, 2016, at 7:48 AM, Miguel A. Vico
>     <mvicomoya at nvidia.com <mailto:mvicomoya at 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 <mvicomoya at nvidia.com
>     <mailto:mvicomoya at nvidia.com>>
>     > > >>> Reviewed-by: James Jones <jajones at nvidia.com
>     <mailto:jajones at nvidia.com>>
>     > > >>
>     > > >> 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.
>  

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 "arrrrgh who wrote this stupid bug") and
provide churn for very little benefit, imho.

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.

(Sorry to sidetrack your thread Miguel!  None of these complaints apply
to you at all!)

Thanks,
Derek

> 
>     Thanks,
>     pq
>     _______________________________________________
>     wayland-devel mailing list
>     wayland-devel at lists.freedesktop.org
>     <mailto:wayland-devel at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>