[vlc-devel] [PATCH] qt: medialib: add async task handle
Pierre Lamot
pierre at videolabs.io
Wed Jan 6 16:47:58 UTC 2021
On 2021-01-06 17:16, Romain Vimont wrote:
> 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.
Hum, I was assuming that the reset(new T) may lead to memory leaks in
some corner cases (exception thrown from the CTor) and that make_unique
was the prefered way, turns out I was wrong
https://stackoverflow.com/questions/40991687/stdmake-uniquet-vs-resetnew-t/41007104
so yes, the reset is probably better in this case
>>
>> > + 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 calls the operator* function, this is pretty much equivalent.
>
> (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.
I'd would favor the get form for consitency with the rest of the code
though It doesn't really matters.
>>
>> > 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list