Mir

Code review comment for lp:~raof/mir/check-for-surfaceless

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

578 +mg::SurfacelessEGLContext::SurfacelessEGLContext(

587 +mg::SurfacelessEGLContext::SurfacelessEGLContext(
588 + EGLDisplay egl_display,
589 + EGLint const* attribs,
590 + EGLContext shared_context)

It would be clearer to implement the SurfaceEGLContext(egl_display, shared_context) constructor in terms of the new SurfaceEGLContext(egl_display, attribs, shared_context) constructor, by passing our default config attribs:

SurfacelessEGLContext(EGLDisplay egl_display, EGLContext shared_context)
 : SurfacelessEGLContext(egl_display, default_attribs, shared_context)

429 +mgo::DisplayBuffer::DisplayBuffer(SurfacelessEGLContext &&egl_context,
430 geom::Rectangle const& area)
431 - : egl_context{std::move(egl_context)},
432 + : egl_context{std::forward<SurfacelessEGLContext>(egl_context)},

It's better to accept a move only type (such as SurfacelessEGLContext or std::unique_ptr<>) by value vs rvalue reference: see https://code.launchpad.net/~rocket-scientists/mir/buffer-swapper-multi/+merge/139127/comments/301713 .

Also, with this code, since the egl_context parameter is an rvalue reference, not a universal reference, using std::forward doesn't offer any benefits over using std::move.

613 + move.egl_display = EGL_NO_DISPLAY;

618 + release_current();

A invalidated (i.e. moved) SurfacelessEGLContext object may try to release the EGL context when being destroyed. Although this will have no effect since egl_display == EGL_NO_DISPLAY, I think it would be better to not try at all to avoid error/debug messages from EGL.

review: Needs Fixing

« Back to merge proposal