Hi Emil, On 15 September 2017 at 11:29, Emil Velikov <[email protected]> wrote: > Currently we're in a bit of a pickle - both from vendor and user POV.
This commit message is a bit long-winded. Could you please try to compress it down to something like: Currently the client-facing libwayland-egl API is defined by a header file shipped by Wayland, but the implementation is left to each vendor. This can cause collisions when multiple implementations are installed on the same system. Importing the implementation into Wayland with a stable and versioned driver-facing ABI allows multiple drivers etc etc Apart from the below, this series, when squashed into a single patch for import, is: Reviewed-by: Daniel Stone <[email protected]> I realise this is a straight copy/import, so sorry to pick on these, but: > +#include <stddef.h> // offsetof > +#include <stdio.h> // printf > + > +#include "wayland-egl-priv.h" // Current struct wl_egl_window implementation C++ comments :( > +/* GCC visibility */ > +#if defined(__GNUC__) > +#define WL_EGL_EXPORT __attribute__ ((visibility("default"))) > +#else > +#define WL_EGL_EXPORT > +#endif This bit can be deleted. wayland-egl-priv.h could be renamed wayland-egl-backend.h, since it's by definition no longer private. I'm also not a fan of importing dead code and only wiring it up later. > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#define WL_EGL_WINDOW_VERSION 3 > + > +struct wl_egl_window { > + const intptr_t version; > + > + int width; > + int height; > + int dx; > + int dy; > + > + int attached_width; > + int attached_height; > + > + void *private; > + void (*resize_callback)(struct wl_egl_window *, void *); > + void (*destroy_window_callback)(void *); > + > + struct wl_surface *surface; > +}; I'd like to see this and the definitions of wayland-egl-abi-check.c not be a complete copy of each other; perhaps in a separate private header? wayland-egl-priv.h could be renamed wayland-egl-backend.h, since it's by definition no longer private. I'm also not a fan of importing dead code and only wiring it up later. > diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check > new file mode 100755 > index 0000000..e7105ea > --- /dev/null > +++ b/egl/wayland-egl-symbols-check > @@ -0,0 +1,16 @@ > +#!/bin/sh > + > +FUNCS=$(nm -D --defined-only ${1-.libs/libwayland-egl.so} | grep -o "T .*" | > cut -c 3- | while read func; do Please take the input library name from the command line, so build-system details are confined to just the build system. > +WL_EGL_EXPORT void > +wl_egl_window_resize(struct wl_egl_window *egl_window, Reuse WL_EXPORT. > +WL_EGL_EXPORT struct wl_egl_window * > +wl_egl_window_create(struct wl_surface *surface, > + int width, int height) > +{ > + struct wl_egl_window _INIT_ = { .version = WL_EGL_WINDOW_VERSION }; > + struct wl_egl_window *egl_window; > + > + if (width <= 0 || height <= 0) > + return NULL; > + > + egl_window = malloc(sizeof *egl_window); > + if (!egl_window) > + return NULL; > + > + memcpy(egl_window, &_INIT_, sizeof *egl_window); The initialiser is weird; it should be static const if it remains, but why not just use calloc and set it like the rest of the members below? Cheers, Daniel _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
