[PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create
Miguel Angel Vico
mvicomoya at nvidia.com
Thu May 12 15:20:49 UTC 2016
Thanks Derek.
Inline.
On Thu, 12 May 2016 08:54:26 -0500
Derek Foreman <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> wrote:
> >
> >> On May 11, 2016, at 7:48 AM, Miguel A. Vico <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>
> >>> Reviewed-by: James Jones <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.
>
> > 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->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 at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >>
> >
> >
>
--
Miguel
More information about the wayland-devel
mailing list