D16555: Move construction of ExpectedSignal where it belongs

[Daniel Vrátil]

dvratil requested changes to this revision.
dvratil added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> entitytreemodeltest.cpp:160
> +        // arrive, dataChanged is emitted for them.
> +        ExpectedSignal{RowsAboutToBeInserted, 0, 0},
> +        ExpectedSignal{RowsInserted, 0, 0},

You may even be able to get rid of the type name, as that can be deduced from the initializer list type, or not?

> entitytreemodeltest.cpp:556
>  
> -    QList<ExpectedSignal> expectedSignals;
> -
> -    expectedSignals << getExpectedSignal(RowsAboutToBeRemoved, sourceRow, sourceRow, sourceCollection, QVariantList() << removedItem);
> -    expectedSignals << getExpectedSignal(RowsRemoved, sourceRow, sourceRow, sourceCollection, QVariantList() << removedItem);
> +    QList<ExpectedSignal> expectedSignals {
> +        ExpectedSignal{RowsAboutToBeRemoved, sourceRow, sourceRow, sourceCollection, QVariantList{removedItem}},

`const`

> modelspy.h:46
> +    ExpectedSignal(SignalType type, int start, int end, QVariant parentData = QVariant(), QVariantList newData = QVariantList())
> +        : signalType{type}, startRow{start}, endRow{end}, parentData{std::move(parentData)}, newData{std::move(newData)}
> +    {

The move here is kinda pointless: Qt classes are implicitly shared, so copying them means only copying a pointer. And if you make the arguments `const &`, calling the ctor would be a no-copy and initialization of the member variable would be a copy. Your approach requires a copy to call the ctor and then move for initialization - so copy vs copy+move: `const &` wins here :)

And while this may sound contradictory to what I replied to your email regarding performance compromises when refactoring, the `std::move` does not IMO improve code readability or quality (compared to just passing non-POD args as `const &` ) in this case.

> tagmodeltest.cpp:176
>  
> +    QList<ExpectedSignal> expectedSignals {
> +        ExpectedSignal{RowsAboutToBeInserted, newRow, newRow, parentTag, QVariantList{addedTag}},

`const`

> tagmodeltest.cpp:221
>  
> -    QList<ExpectedSignal> expectedSignals;
> -
> -    expectedSignals << getExpectedSignal(DataChanged, changedRow, changedRow, parentTag, QVariantList() << tagName);
> -
> +    QList<ExpectedSignal> expectedSignals {
> +        ExpectedSignal{DataChanged, changedRow, changedRow, parentTag, QVariantList{tagName}}

`const`

> tagmodeltest.cpp:269
> +    const auto parentTagVariant = parentTag.isEmpty() ? QVariant{} : parentTag;
> +    QList<ExpectedSignal> expectedSignals {
> +        ExpectedSignal{RowsAboutToBeRemoved, sourceRow, sourceRow, parentTagVariant, QVariantList{removedTag}},

`const`

> tagmodeltest.cpp:329
> +    const auto newParentVariant = newParent.isEmpty() ? QVariant{} : newParent;
> +    QList<ExpectedSignal> expectedSignals {
> +        ExpectedSignal{RowsAboutToBeMoved, sourceRow, sourceRow, parentTagVariant, destRow, newParentVariant, QVariantList{changedTag}},

`const`

REPOSITORY
  R165 Akonadi

REVISION DETAIL
  https://phabricator.kde.org/D16555

To: dkurz, #kde_pim, dvratil
Cc: dvratil, kde-pim, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20181101/785c5342/attachment.html>