Mir

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

Revision history for this message
Brandon Schaefer (brandontschaefer) 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()

Dont see this namespace used in the header.
+class Session;

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
      { }

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 :)

+ if (!((the_params.content_id.is_set()) ||
+ (the_params.streams.is_set() && the_params.streams.value().size() > 0

Would turn into:

if (!the_params.content_id.is_set() && (the_params.streams.is_set() || the_params.streams.value().size() > 0))

Up to you :)

« Back to merge proposal