Use std::expected for outcome when available

[jb-gcx]

Motivation

Using std types when possible increases compatibility with other codebases.

Considerations

• I've removed make_unexpected because it is unnecessary in my environment. Are there issues on other compilers with type deduction for unexpected?
• I tested the code with real devices on iOS and Android
• I was unable to test the code with a compiler that does not support std::expected (ie does not define the feature flag). I can offer a godbolt link to show the basic feature check works (just switch to -std=c++20 to see that supported changes to false). I can probably create a setup to test this properly, but if someone else already has such a setup I would greatly appreciate some support.
• I did not test the wasm and ts adapters.
• This changes behavior of existing code in environments where std::expected is available. I'd argue it does so in unproblematic ways:
  • The codebase used djinni::expected everywhere - the change should be unnoticable.
  • The codebase used std::expected in some places and djinni::expected in others. This change would make any conversion code between the two types obsolete, but should not break anything, since djinni::expected now just maps to std::expected.
  • The codebase used tl::expected in some places and djinni::expected in others. This is the only problematic case, though the solution is simple: use std::expected or djinni::expected.

[li-feng-sc]

Thanks for your contribution. I think make_unexpected() used to be necessary to make it work in C++14. Class template argument deduction is a C++17 feature. Snap's codebase is currently at C++20 so I think it is fair now to drop C++14 support.

[jb-gcx]

Sounds good, thanks for the feedback 👍

Let me know if there is anything else for me to do to get this merged.

[li-feng-sc]

<version> is a c++20 header. Can we somehow maintain compatibility with c++17?

[li-feng-sc]

do you think we can guard it with __has_include?

[paulocoutinhox]

Hi,

Please, you can use something like this:

#if __has_include(<version>)
#include <version>
#endif

It will make compatible with c++17.

I tested on godbolt with some compilers and it works.

Thanks.

[jb-gcx]

Thank you for the feedback, this makes sense. I've applied the change suggested by @paulocoutinhox and also added a #ifdef __has_include before it, because this should make it compatible with even earlier compilers. I got the idea from the example in https://en.cppreference.com/w/cpp/feature_test , which is apparently even C++11 compatible.

[paulocoutinhox]

The code from cppreference is:

#ifdef __has_include
# if __has_include(<version>)
#   include <version>
# endif
#endif

Can be this too.

[jb-gcx]

You mean the indentations? I don't usually use that style, so I removed it without thinking about it.

I'm trying to figure out what's usually used in this codebase. Looking at DataRef.hpp and Future.hpp, there are sometimes indentations for precompiler directives, but not always. And the indentations are sometimes 2, sometimes 4 spaces. And the indentations are always before the pound sign, never after. I'll happily change the indentations if someone (@LiFengSC ?) can tell me what the preferred style is.

[paulocoutinhox]

No, i mean the two directives for secure include it.

First is checking if exists __has_include: #ifdef __has_include
Second is checking the version header: #if __has_include(<version>)

[jb-gcx]

The code I pushed earlier is already doing that:

#ifdef __has_include
#if __has_include(<version>)
#include <version>
#endif
#endif

Or am I missing something obvious?

[paulocoutinhox]

No, you are correct, i don't see that you add #ifdef __has_include. Sorry, my mistake.

[li-feng-sc]

One last request: Lets keep the make_unexpected helper function in both code paths:

#ifdef __cpp_lib_expected

#include <expected>

namespace djinni {

using ::std::expected;
using ::std::unexpected;

template <class E>
unexpected<typename std::decay<E>::type> make_unexpected(E &&e) {
    return unexpected {std::forward<E>(e)};
}

}

#else

#include "tl_expected.hpp"

namespace djinni {

using ::tl::unexpected;
using ::tl::expected;
using ::tl::make_unexpected;

}

#endif

the make_unexpected function is already used in many places in Snap's codebase. If we keep it then it mean there is zero change needed on user side. We can also remove the changes in the marshallers in this PR.

C++20 code can directly use unexpected constructor and ignore the helper function.

[jb-gcx]

I saw your comment via eMail and didn't realize you had posted code until I pushed my changes and checked the PR. I hope my way of reintroducing the helper is also acceptable?

I also added the type_traits and utility headers, because they're technically required for std::forward and std::decay. Although they seem to already have been included transitively anyway.

I've force pushed to avoid merging fixup commits, I think the changes are small enough that this won't cause confusion.

[li-feng-sc]

No problem, I like the way you did it.