Mir

Code review comment for lp:~cemil-azizoglu/mir/improve-raii

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

===
68 +typedef std::function<void(graphics::Buffer* buffer)> release_callback;
===

Should be ReleaseCallback according to the mir style guide:
http://unity.ubuntu.com/mir/cppguide/index.html#Type_Names

===
80 + virtual ~BufferHandle();
===

Not needed

===
356 + if (release_fn)
357 + release_fn(wrapped.get());
===
I think Move assignment should not cause the buffer to be released back to the queue.
That would be analogous to move assigning a unique_ptr and having the ptr be deleted while doing that.

===
1198 + compositor_buffer_handle{nullptr, nullptr},
1209 + if (!compositor_buffer_handle.buffer())
1210 + compositor_buffer_handle = underlying_buffer_queue->compositor_acquire(compositor_id);
===

Maybe a bit cleaner (but not blocking) if you allow BufferHandle to be default constructed and implement the negation operator,
so that line 1198 is not needed then, and line 1209 just becomes: if (!compositor_buffer_handle)

===
1623 + return std::move(compositor::BufferHandle(stub_compositor_buffer, nullptr));
1628 + return std::move(compositor::BufferHandle(stub_compositor_buffer, nullptr));
===

You ca do away with the std::move

Needs information:

Do the BufferStream integration and unit tests (BufferStreamTest.*) overlap with other tests?
Otherwise they should be updated but kept not removed.

review: Needs Fixing

« Back to merge proposal