Thanks Derek. Inline.
On Thu, 12 May 2016 08:54:26 -0500 Derek Foreman <der...@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 <j...@humanoriented.com> wrote: > > > >> On May 11, 2016, at 7:48 AM, Miguel A. Vico <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 <mvicom...@nvidia.com> > >>> Reviewed-by: James Jones <jajo...@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@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel > >> > > > > > -- Miguel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
