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

util: Add tl::expected type #2317

Merged
merged 2 commits into from
Jul 9, 2023
Merged

Conversation

CohenArthur
Copy link
Member

Directly taken from https://github.com/TartanLlama/expected and adapted
slightly for our use case.

gcc/rust/ChangeLog:

* util/expected.h: New file.

This type offers a type similar to std::expected, which is only available since C++23. It will be useful for cleaning up our error handling, chaining errors together, and easier control flow in general.

This will be used for the upcoming experimental name resolution algorithm.

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

Added in other GCC folks, are we allowed to do this in GCC?

Another option might be to take your optional implementation and reuse it to bootstrap a Result<Value, Error> class helper.

@philberty
Copy link
Member

I am not sure how i feel about it my initial thought is we might have issues using this with GCC but on the other hand it could be a useful interface.

@dkm and @tschwinge any opinion?

@tschwinge
Copy link
Member

This has recently been discussed in GCC/Rust IRC, 2023-06-15: https://github.com/Rust-GCC/gccrs/wiki/IRC-2023-06-15.

@tschwinge
Copy link
Member

This has recently been discussed in GCC/Rust IRC, 2023-06-15: https://github.com/Rust-GCC/gccrs/wiki/IRC-2023-06-15.

The outcome of that -- per my free interpretation -- was something like this:

Should talk to libstdc++ maintainers to determine whether doing this via existing (or future) libstdc++ code is feasible, whilst maintaining GCC 4.8 compatibility, and otherwise it's deemed OKish to import external code.

@tschwinge
Copy link
Member

One thing that's going to be useful for future maintenance: first import the external file(s) verbatim, and then do local changes in separate commit(s). For some (?) external imports, LOCAL_PATCHES files are maintained -- not sure whether that'll be useful here, too?

$ find -name LOCAL_PATCHES
./libffi/LOCAL_PATCHES
./libsanitizer/LOCAL_PATCHES
./libstdc++-v3/src/c++17/fast_float/LOCAL_PATCHES
./libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES

@CohenArthur
Copy link
Member Author

One thing that's going to be useful for future maintenance: first import the external file(s) verbatim, and then do local changes in separate commit(s). For some (?) external imports, LOCAL_PATCHES files are maintained -- not sure whether that'll be useful here, too?

$ find -name LOCAL_PATCHES
./libffi/LOCAL_PATCHES
./libsanitizer/LOCAL_PATCHES
./libstdc++-v3/src/c++17/fast_float/LOCAL_PATCHES
./libstdc++-v3/src/c++17/ryu/LOCAL_PATCHES

@tschwinge this is going to create commits that do not compile, as the original implementation uses exceptions which we explicitly disable. So I'm not sure how needed this is?

@tschwinge
Copy link
Member

this is going to create commits that do not compile, as the original implementation uses exceptions which we explicitly disable.

There's no such problem: the file will be unused initially. :-)

@CohenArthur
Copy link
Member Author

this is going to create commits that do not compile, as the original implementation uses exceptions which we explicitly disable.

There's no such problem: the file will be unused initially. :-)

ah, that's a good point! :D

@CohenArthur
Copy link
Member Author

I'll split up the commit then

Directly taken from https://github.com/TartanLlama/expected.

gcc/rust/ChangeLog:

	* util/expected.h: New file.
@CohenArthur CohenArthur force-pushed the expected-type branch 2 times, most recently from bdbf94e to 986ff27 Compare June 29, 2023 09:59
Disable exceptions and remove inclusion of poisoned headers.

gcc/rust/ChangeLog:

	* util/expected.h: Adapt class to GCC requirements.
@CohenArthur CohenArthur added this pull request to the merge queue Jul 9, 2023
Merged via the queue into Rust-GCC:master with commit 44ffe11 Jul 9, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants