[vlc-devel] [PATCH] qt: medialib: add async task handle

[Romain Vimont]

Thank you for the review. Comments inline.

Regards

On Wed, Jan 06, 2021 at 04:55:36PM +0100, Pierre Lamot wrote:
> Hi, OK for the patch,
> 
> some nits bellow.
> 
> On 2021-01-06 15:55, Romain Vimont wrote:
> > Use a smart pointer to "abandon" an AsyncTask using RAII. This avoids to
> > forget to call abandon() on client destruction.
> > ---
> >  modules/gui/qt/util/asynctask.hpp | 22 +++++++++++++++++++
> >  modules/gui/qt/util/listcache.hpp | 36 +++++++++----------------------
> >  2 files changed, 32 insertions(+), 26 deletions(-)
> > 
> > diff --git a/modules/gui/qt/util/asynctask.hpp
> > b/modules/gui/qt/util/asynctask.hpp
> > index fa4a6d3b2c..81fcc54022 100644
> > --- a/modules/gui/qt/util/asynctask.hpp
> > +++ b/modules/gui/qt/util/asynctask.hpp
> > @@ -128,6 +128,9 @@
> >   * to declare that it won't use it anymore (and don't want the result).
> > When
> >   * both conditions are met, the task is deleted via
> > `QObject::deleteLater()`.
> >   *
> > + * For convenience, a `TaskHandle` smart pointer is provided to call
> > + * `abandon()` using RAII.
> > + *
> >   * The task result is provided via a separate method `takeResult()`
> > instead of
> >   * a slot parameter, because otherwise, Qt would require its type to
> > be:
> >   *  - non-template (like `std::vector<T>`),
> > @@ -243,4 +246,23 @@ private:
> >      AsyncTask<T> *m_task;
> >  };
> > 
> > +template <typename T>
> > +struct TaskDeleter
> > +{
> > +    void operator()(T *task) { task->abandon(); }
> > +};
> > +
> > +/**
> > + * Task handle to abandon() a task using RAII
> > + *
> > + * It behaves like a unique_ptr, but its deleter calls
> > AsyncTask<T>::abandon(),
> > + * which indicates that the client cancels the task and requests
> > deletion when
> > + * possible.
> > + *
> > + * The actual deletion may happen later (when the runnable also
> > finished its
> > + * work).
> > + */
> > +template <typename T>
> > +using TaskHandle = std::unique_ptr<T, TaskDeleter<T>>;
> > +
> >  #endif
> > diff --git a/modules/gui/qt/util/listcache.hpp
> > b/modules/gui/qt/util/listcache.hpp
> > index e92b7d84df..20fa510832 100644
> > --- a/modules/gui/qt/util/listcache.hpp
> > +++ b/modules/gui/qt/util/listcache.hpp
> > @@ -115,7 +115,6 @@ public:
> >          : m_threadPool(threadPool)
> >          , m_loader(loader)
> >          , m_chunkSize(chunkSize) {}
> > -    ~ListCache();
> > 
> >      /**
> >       * Return the item at specified index
> > @@ -167,19 +166,10 @@ private:
> >      bool m_countRequested = false;
> >      MLRange m_lastRangeRequested;
> > 
> > -    LoadTask<T> *m_loadTask = nullptr;
> > -    CountTask<T> *m_countTask = nullptr;
> > +    TaskHandle<LoadTask<T>> m_loadTask;
> > +    TaskHandle<CountTask<T>> m_countTask;
> >  };
> > 
> > -template <typename T>
> > -ListCache<T>::~ListCache()
> > -{
> > -    if (m_countTask)
> > -        m_countTask->abandon();
> > -    if (m_loadTask)
> > -        m_loadTask->abandon();
> > -}
> > -
> >  template <typename T>
> >  const T *ListCache<T>::get(size_t index) const
> >  {
> > @@ -248,8 +238,8 @@ void ListCache<T>::asyncCount()
> >  {
> >      assert(!m_countTask);
> > 
> > -    m_countTask = new CountTask<T>(m_loader);
> > -    connect(m_countTask, &BaseAsyncTask::result,
> > +    m_countTask.reset(new CountTask<T>(m_loader));
> 
> maybe use std::make_unique

std::make_unique creates a new std::unique_ptr instance. Here, that
would create a new instance just to move it (using the move-assignment
operator) into m_countTask. Calling reset() is more straightforward IMO.

> 
> > +    connect(&*m_countTask, &BaseAsyncTask::result,
> >              this, &ListCache<T>::onCountResult);
> 
> m_countTask.get() directly gives you the underlying address.

Yes, I used &* because it gives the underlying address without calling
get() ;)

(It's often used in Rust for example to get a &T from a Box<T>
without calling .as_ref()).

If you prefer .get(), I'm totally ok with that.

> 
> >      m_countRequested = true;
> >      m_countTask->start(m_threadPool);
> > @@ -259,15 +249,14 @@ template <typename T>
> >  void ListCache<T>::onCountResult()
> >  {
> >      CountTask<T> *task = static_cast<CountTask<T> *>(sender());
> > -    assert(task == m_countTask);
> > +    assert(task == &*m_countTask);
> > 
> >      m_offset = 0;
> >      m_list.clear();
> >      m_total_count = static_cast<ssize_t>(task->takeResult());
> >      emit localSizeChanged(m_total_count);
> > 
> > -    task->abandon();
> > -    m_countTask = nullptr;
> > +    m_countTask.reset();
> >  }
> > 
> >  template <typename T>
> > @@ -298,12 +287,8 @@ private:
> >  template <typename T>
> >  void ListCache<T>::asyncLoad(size_t offset, size_t count)
> >  {
> > -    if (m_loadTask)
> > -        /* Cancel any current pending task */
> > -        m_loadTask->abandon();
> > -
> > -    m_loadTask = new LoadTask<T>(m_loader, offset, count);
> > -    connect(m_loadTask, &BaseAsyncTask::result,
> > +    m_loadTask.reset(new LoadTask<T>(m_loader, offset, count));
> > +    connect(&*m_loadTask, &BaseAsyncTask::result,
> >              this, &ListCache<T>::onLoadResult);
> >      m_lastRangeRequested = { offset, count };
> >      m_loadTask->start(m_threadPool);
> > @@ -313,15 +298,14 @@ template <typename T>
> >  void ListCache<T>::onLoadResult()
> >  {
> >      LoadTask<T> *task = static_cast<LoadTask<T> *>(sender());
> > -    assert(task == m_loadTask);
> > +    assert(task == &*m_loadTask);
> > 
> >      m_offset = task->m_offset;
> >      m_list = task->takeResult();
> >      if (m_list.size())
> >          emit localDataChanged(m_offset, m_list.size());
> > 
> > -    task->abandon();
> > -    m_loadTask = nullptr;
> > +    m_loadTask.reset();
> >  }
> > 
> >  #endif
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel