Snapchat / djinni

A tool for generating cross-language type declarations and interface bindings. Djinni's new home is in the Snapchat org.
Apache License 2.0
166 stars 44 forks source link

Use std::expected for outcome when available #166

Closed jb-gcx closed 2 months ago

jb-gcx commented 3 months ago

Motivation

Using std types when possible increases compatibility with other codebases.

Considerations

LiFengSC commented 3 months ago

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 commented 3 months ago

Sounds good, thanks for the feedback 👍

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

LiFengSC commented 3 months ago

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 commented 3 months ago

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.

LiFengSC commented 2 months ago

No problem, I like the way you did it.