Implement QPromise<T>::convert<U>()

[dpurgin]

Hi Simon,

I've implemented an initial version of QPromise<T>::as<U>() for casting arithmetic types, conversion to void, conversion of QVariant and copy-ctor calls for user-defined types.

Could you take a look please? I'll add the documentation and examples as soon as the implementation is done

Cheers

[simonbrunel]

Thanks @dpurgin, it looks good.

I'm wondering if it makes sense to have 2 methods:

• QPromise::convert<U>(): the behavior implemented in this PR.
• QPromise::cast<U>() or ::as<U>(): only static_cast<U> without trying to convert the original value. No QVariant involved and it would not compile if the cast is not possible. I think we could optimize it by casting the internal shared pointer only.

I'm really not sure which approach is better (1 vs 2 methods), what do you think?

nit: if it's not already the case, make sure to run clang-format.

[simonbrunel]

What does this additional copyright line actually mean?

[dpurgin]

I've modified a substantial portion of the file (the 'substantiality' is of course subjective, but, in fact, this file has been added by me), so the line refers to this file. Would you prefer another format to address the others? Like one line 'Simon Brunel and contributors'?

[simonbrunel]

I understand why you added it only in this file, but what does that mean legally at the file level but also at the project level?

Anyway, I don't want to have to maintain different copyright in each file and keep track of who created a file or who changed most of it. The contribution history is available in git and easily accessible in many places in GitHub (contributors, blame, etc.). I also want the same header in all files but I'm not sure yet how I want to change it. Let me think about it, however I would like to make this change in a separate commit outside this PR.

[dpurgin]

Hey Simon, thanks for the review.

My Qt Creator runs clang-format automatically upon editing, so this shouldn't be a problem, but I'll make sure I reformat my changes again.

As for two separate methods convert and cast/as, I'm a bit unsure. On one side, this makes clear that the intention is to convert a variant (we could also add std::variant or std::any) and to cast otherwise. On the other hand, since both words are kind of synonyms, this could lead to a confusion: one would have to remember, what the difference between convert and cast is, whether cast is dynamic, static, or reinterpret. In the original problem where I wanted to just drop the fulfilled value and convert to QPromise<void>, I think I would be wondering whether I should call convert or cast if I'm a new user.

Generally, I support the separation of the functionality into convert and cast but I also think it could be too much of an API for a simple procedure. We could probably implement two methods and deprecate one of them if it causes a lot of questions.

[simonbrunel]

I'm not sure there is a use case for calling the cast method instead of the conversion one. And I agree that one method should be enough and easier to understand. If someone really wants to avoid calling the conversion API, it's still easy to write:

p.then([](T& value) { 
    return static_cast<U>(value)
});

[dpurgin]

Simon, could you please review the latest update:

• Converters are moved to qpromise_p.h, their implementations return a conversion lambda
• Support for void<->QVariant
• U{T} removed, static_cast<U>(T) is the primary converter
• Copyright header in tst_as.cpp is the same as in all other files
• Some more tests added to cover all convertors.

I'm not sure about testing the other QVariant conversions with built-in Qt types like in your branch. They all call the same converter and, ultimately, we will be testing the class QVariant instead of QtPromise. I understand that formally it is a black box testing procedure and all these tests would make sense, but then there is a question how many Qt types should be tested, how do we pick them and where to stop?

If the behavior and the API of the PR are ok, I'll start with the docs.

Cheers

[simonbrunel]

They all call the same converter and, ultimately, we will be testing the class QVariant instead of QtPromise.

I agree, current tests should be enough. If a bug for a specific type is reported, then we will add a regression test for it. Though, I will need to take another look at your tests since I don't have time to review it now.

If the behavior and the API of the PR are ok, I'll start with the docs.

I left a comment about void -> QVariant which should be the only one to impact the docs.

Thanks @dpurgin

[dpurgin]

Hey Simon, could you review the last changes please

• docs added
• void -> QVariant conversion removed, std::enable_if removed as well
• PromiseConverter<T, void> removed

Cheers

[simonbrunel]

Really good work!

Most of my comments are minor changes.

While reading the docs, I feel that .as() is a mix of the .cast() vs .convert() APIs and I start to think it provides an inconsistent behavior because it allows to convert QVariant(string) -> int but not string -> int (and vice-versa).

Alternative behaviors would be:

1. don't support converting QVariant at all (so only rely on static_cast).
2. only convert QVariant if the original value is of the same type as U (not sure it's possible).
3. or support the same conversion logic as QVariant.

(1) looks the most consistent. I don't see a good use case for (2) while (3) could be interesting as a real conversion API. Though, the current implementation is probably not an issue, we can still split it in .cast & .convert later (and deprecate .as) if it's confusing / problematic to some users.

Thoughts?

[simonbrunel]

Solution 4: have an explicit API that converts the value as described in (3) and support simple static_cast via assignment operator / copy constructors:

// static_cast
QPromise<int> p0 = QtPromise::resolve(42);
QPromise<double> p1 = p0;
QPromise<void> p2 = p1;
QPromise<void> p3 = QtPromise::all<void>({p0, p1, p2});

// QPromise<QString> p4 = p0; // FAIL

// convert
QPromise<QString> p5 = p0.as<QString>();

[dpurgin]

Hey @simonbrunel,

could you please review the changes as discussed on Slack:

• as<U>() renamed to convert<U>() and moved from QPromiseBase<T> to QPromise<T>
• Added a "generic" conversion via QVariant

I've specialized the static_cast specialization not only for arithmetic types, but also for types having a converting constructor and for enums as well. Unfortunately, there is no one single compile-time check to flag such types, so I've ended up with a series of conditions. On the bright side, I've managed to dispatch it without the enable_if stuff.

Cheers

[simonbrunel]

Thank you @dpurgin, really good work!

I didn't know about QMetaType::registerConverter, that's really great to extend the QPromise conversion API. That could be also something to investigate in order to implicitly convert a QPromise to a QML promise (and vice-versa) (i.e. related to #5 / this branch).

Indeed, nice trick to avoid duplicating the std::enable_if logic. I'm still not fan of the code layout it produces, but I can figure out later how to make it looks nicer - or just live with it :)

I left a few comments, all minors.

[dpurgin]

Hi @simonbrunel,

thanks for the comments! I've fixed them in the recent commits.

Cheers

[simonbrunel]

Thank you @dpurgin for your work!