Mir

Code review comment for lp:~kdub/mir/require-streams-when-creating-surface

Revision history for this message
Kevin DuBois (kdub) wrote :

> Nit. Couldnt a_surface() take a mir::graphics::BufferProperties? Since we use
> all the same arguments in both. Such as of_buffer_properties(...) or
> something... though either way we have to use those values twice :)
> + auto params = ms::a_surface()
>

Surfaces don't really have pixel_format and usage, these are really just properties of buffers. (although surfaces do have pf/usage fields still from un-cleaned-up cruft).

> Dont see this namespace used in the header.
> +class Session;
This is needed in the remove_content_for() function added to the header

> Shared ptr are defaulted to a nullptr, no need to set it.
> + std::shared_ptr<mf::BufferStream> legacy_stream = nullptr;
>
> // /usr/include/c++/5/tr1/shared_ptr.h:546
> __shared_ptr()
> : _M_ptr(0), _M_refcount() // never throws
> { }

fixed

> This could be a bit simpler, as you have right now:
> !(A || (B && C))
> This could turn into:
> !A && (B || C)
>
> (wanted to do a truth table just to check haha:
> http://paste.ubuntu.com/15635820/)
>
> Not super sure if its *simpler* just reads a bit easier :)
>

Line 6 in the pastebin isn't correct. (the original expression is not computed correctly, the new expression is)

The expression could be reduced I suppose:
!(A || (B && C))
to
!A && !(B && C)
to
!A && (!B || !C)

« Back to merge proposal