Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backport c++23 std::expected #40

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

zhjwpku
Copy link
Collaborator

@zhjwpku zhjwpku commented Jan 26, 2025

After studied [0], [1] and [2], I'm inclined to backport [0] to iceberg-cpp, the main reason is that [0] has exactly the same APIs as std::expected, [1] provide some extra member functions like map and map_error, [2] has different API names like then and thenOrThrow.

We want users to use iceberg::expected the same way as std::expected, and we will replace iceberg::expected with std::expected when we decide to move to c++23 some day, by backporting [0], we can facilitate a smoother transition process.

We discussed about Exceptions vs Expected in #14, while backporting [0], I had the feeling we shouldn't choose one over the other, we can use both approaches effectively.

expected provide monadic operations like and_then, transform, or_else, transform_error, let us do method chaining, which is a good sign.

[0] https://github.com/zeus-cpp/expected
[1] https://github.com/TartanLlama/expected
[2] https://github.com/facebook/folly/blob/main/folly/Expected.h
[3] https://en.cppreference.com/w/cpp/utility/expected

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to update NOTICE and/or LICENSE when vendoring code?

After studied [0], [1] and [2], I'm inclined to backport [0] to
iceberg-cpp, the main reason is that [0] has exactly the same
APIs as std::expected, [1] provide some extra member functions
like `map` and `map_error`, [2] has different API names like
`then` and `thenOrThrow`.

We want users to use iceberg::expected the same way as std::expected,
and we will replace iceberg::expected with std::expected when
we decide to move to c++23 some day, by backporting [0], we can
facilitate a smoother transition process.

We discussed about `Exceptions vs Expected` in apache#14, while
backporting [0], I had the feeling we shouldn't choose one over
the other, we can use both approaches effectively.

expected provide monadic operations like `and_then`, `transform`,
`or_else`, `transform_error`, let us do method chaining, which
is a good sign.

[0] https://github.com/zeus-cpp/expected
[1] https://github.com/TartanLlama/expected
[2] https://github.com/facebook/folly/blob/main/folly/Expected.h
[3] https://en.cppreference.com/w/cpp/utility/expected

Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
disable clang-tidy for backported std:expected since we don't want
to diverge from upstream.

Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. IMO let's get moving on this.

BTW, we could consider writing a custom check to just enforce [[nodiscard]] on functions returning expected? https://clang.llvm.org/extra/clang-tidy/Contributing.html

Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
@zhjwpku
Copy link
Collaborator Author

zhjwpku commented Feb 4, 2025

LGTM. IMO let's get moving on this.

BTW, we could consider writing a custom check to just enforce [[nodiscard]] on functions returning expected? https://clang.llvm.org/extra/clang-tidy/Contributing.html

Reasonable, will file an issue once this PR got merged.

@wgtmac
Copy link
Member

wgtmac commented Feb 5, 2025

Is it possible to check the macro __cpp_lib_expected and directly use std::expected if available?

@pitrou
Copy link
Member

pitrou commented Feb 5, 2025

Is it possible to check the macro __cpp_lib_expected and directly use std::expected if available?

That may lead to ABI issues. For example if iceberg-cpp is compiled using a C++20 compiler, and then an application links to it with C++23 enabled. Or the reverse.

@wgtmac
Copy link
Member

wgtmac commented Feb 5, 2025

That may lead to ABI issues. For example if iceberg-cpp is compiled using a C++20 compiler, and then an application links to it with C++23 enabled. Or the reverse.

Make sense. Thanks!

@zhjwpku
Copy link
Collaborator Author

zhjwpku commented Feb 6, 2025

@Fokko @Xuanwo I'd appreciate it if you could review this backport of std::expected. While we haven't yet decided whether to use expected over exception, having this backport ready could be beneficial for future feature implementation.

@zhjwpku zhjwpku mentioned this pull request Feb 14, 2025
@wgtmac
Copy link
Member

wgtmac commented Feb 18, 2025

@pitrou @raulcd Do you have any comment on this PR?

@pitrou
Copy link
Member

pitrou commented Feb 18, 2025

I'm surprised by the license header. Shouldn't we use the Apache license there and record the provenance elsewhere?

@zhjwpku
Copy link
Collaborator Author

zhjwpku commented Feb 19, 2025

I'm surprised by the license header. Shouldn't we use the Apache license there and record the provenance elsewhere?

Yes, that was the initial version. I adopted the ASF License and included the zeus-cpp License (MIT) in the NOTICE file. There is a line in the ASF License:

See the NOTICE file distributed with this work for additional information regarding copyright ownership.

I'm unsure if this is sufficient. @lidavidm suggested some references from Arrow, which led us to finalize the current version.

@lidavidm
Copy link
Member

An Apache member corrected this in arrow-java by making sure vendored files had their original license header: apache/arrow-java#550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants