Merge lp:~kdub/mir/update-bufferstream-tracker-todos into lp:mir

[Kevin DuBois]

update the TODO comments in mf::BufferStreamTracker. The first two were TODOs from long ago that don't apply anymore, and the last todo was clarified.

update the TODO comments in mf::BufferStreamTracker. The first two were TODOs from long ago that don't apply anymore, and the last todo was clarified.

[Robert Carr (robertcarr)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2515
http://jenkins.qa.ubuntu.com/job/mir-ci/3629/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2196
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2195
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2146
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1626
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1626/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2146
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2146/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5063
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19952

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3629/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

We're committing to using raw pointers? Are you sure?

17 - /* Access the buffer resource that the id corresponds to.
18 - TODO: should really be a weak or shared ptr */
19 + /* Access the buffer resource that the id corresponds to. */
20 graphics::Buffer* buffer_from(graphics::BufferID) const;

review: Needs Information

[Kevin DuBois (kdub)]

It came up on irc yesterday that the comments are out-of-date. It seems like using a smart pointer (or a moveable resource containing the buffer perhaps) might be better, but I'm not aware of any drive to revert from the raw-ptr interfaces that we have now.

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Daniel van Vugt (vanvugt)]

I think that TODO should remain. In systems as complicated as Mir, raw pointers are scary and a risk...

[Daniel van Vugt (vanvugt)]

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> It seems like using a smart pointer (or a moveable resource containing the buffer perhaps)
> might be better, but I'm not aware of any drive to revert from the raw-ptr interfaces
> that we have now.

> I think that TODO should remain. In systems as complicated as Mir, raw pointers are scary
> and a risk...

Raw *non-owning* pointers are OK in general, but, in any case, the root cause is not in the BufferStreamTracker interface, but rather in the BufferStream/BufferBundle interfaces. BST is forced to use a raw pointer because this is what it gets from BufferStream. If we think that a raw pointer is not sufficient we should have a TODO in BufferStream/BufferBundle.

review: Approve

[Daniel van Vugt (vanvugt)]

Bah, "TODO" or not. A simple grep can find these issues still.

review: Abstain

=== modified file 'src/server/frontend/buffer_stream_tracker.h'
--- src/server/frontend/buffer_stream_tracker.h	2015-04-25 10:45:30 +0000
+++ src/server/frontend/buffer_stream_tracker.h	2015-04-27 20:04:19 +0000
@@ -45,7 +45,7 @@
     /* track a buffer as associated with a buffer stream
      * \warning the buffer must correspond to a single buffer stream id
      * \param buffer_stream_id id that the the buffer is associated with
-     * \param buffer     buffer to be tracked (TODO: should be a shared_ptr)
+     * \param buffer     buffer to be tracked
      * \returns          true if the buffer is already tracked
      *                   false if the buffer is not tracked
      */
@@ -53,17 +53,16 @@
     /* removes the buffer stream id from all tracking */
     void remove_buffer_stream(BufferStreamId);
 
-    /* Access the buffer resource that the id corresponds to.
-       TODO: should really be a weak or shared ptr */
+    /* Access the buffer resource that the id corresponds to. */
     graphics::Buffer* buffer_from(graphics::BufferID) const;
 
 private:
     size_t const client_cache_size;
     std::unordered_map<BufferStreamId, std::shared_ptr<ClientBufferTracker>> client_buffer_tracker;
 
-//TODO: deprecate below this line once exchange_buffer is the normal way to request a new buffer
 public:
     /* accesses the last buffer given to track_buffer() for the given BufferStreamId */
+    //TODO: once next_buffer rpc call is fully deprecated, this can be removed.
     graphics::Buffer* last_buffer(BufferStreamId) const;
 private:
     mutable std::mutex mutex;