optional: Import tl's optional type #2327

CohenArthur · 2023-06-26T12:51:59Z

This is directly adapted from https://github.com/TartanLlama/optional.
This class is tested for C++11 and on GCC 4.8, which is our target mimimum
GCC version.

This optional class provides multiple advantages over our existing
implementation of Optional:

Easier API (no static constructors)
Better method chaining (and_then, map...)
Harsher semantics (no assign-through on Optional<T&>)

Over the std::optional type present in C++'s standard library, this type offers:

Optional references without going through std::reference_wrapper
Better method chaining

For these reasons, I am in favor of importing it directly rather than
adapt C++14's std::optional type to work with C++11 and GCC 4.8.

gcc/rust/ChangeLog:

* util/optional.h: New file.

tschwinge · 2023-06-26T13:53:08Z

#2317 (comment) and following messages apply here, too.

philberty

Goooo for it....

philberty · 2023-06-27T09:30:03Z

gcc/rust/util/optional.h

+static constexpr nullopt_t nullopt{nullopt_t::do_not_use{},
+				   nullopt_t::do_not_use{}};
+
+class bad_optional_access : public std::exception


can this be removed then

philberty · 2023-06-27T09:30:53Z

gcc/rust/util/optional.h

+#define TL_OPTIONAL_VERSION_PATCH 0
+
+// TODO: Are these okay?
+#include <exception>


A bootstrap build will show if it works, I say we should include this into the rust-session-manager and merge it and see how MArks build farm copes with it before merging the other code so we can fix anything we find

IIRC, gcc is built without exceptions https://gcc.gnu.org/codingconventions.html#Exceptions

I think it's okay to modify the file so it includes "rust-system.h" instead of the poisoned headers. As mentioned on the #2317 PR, I'll split up the commit into adding the original file and then modifying it.

This is directly adapted from https://github.com/TartanLlama/optional. This class is tested for C++11 and on GCC 4.8, which is our target mimimum GCC version. This optional class provides multiple advantages over our existing implementation of Optional<T>: - Easier API (no static constructors) - Better method chaining (and_then, map...) - Harsher semantics (no assign-through on Optional<T&>) Over the std::optional type present in C++'s standard library, this type offers: - Optional references without going through std::reference_wrapper - Better method chaining For these reasons, I am in favor of importing it directly rather than adapt C++14's std::optional type to work with C++11 and GCC 4.8. gcc/rust/ChangeLog: * util/optional.h: New file.

tschwinge · 2023-06-28T13:28:52Z

Re commit "optional: Adapt class to GCC's standards": "it also formats the file according to our .clang-format": that's generally frowned upon, as it makes it nearly impossible to see what actually has been changed. Instead, how about making such files exempt from clang-format, via some magic in .github/workflows/clang-format.yml (is there an option to skip/exclude individual files -- or if only directories, put such things into an gcc/rust/imported or similar directory?), or use markers to have clang-format ignore this file, like // clang-format off on top of the file, https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code? (That's however just my opinion/preference).

CohenArthur · 2023-06-28T15:50:06Z

@tschwinge I would rather split this commit in two, then - one to format the file, and one to make the necessary changes. Any option other than adding // clang-format off at the top of the file will cause unaware contributors (like me) to autoformat the file regularly. I'm also in favor of having it formatted

tschwinge · 2023-06-28T16:10:35Z

OK, fair point about local auto-formatting. But why not // clang-format off at the top of the file then, which ought to take care of this issue?

CohenArthur · 2023-06-29T08:46:28Z

Oh, I'm good with that too! But is it okay for GCC?

tschwinge · 2023-06-29T09:40:35Z

is it okay for GCC?

What problem would you anticipate?

CohenArthur · 2023-06-29T09:46:38Z

just that the lack of respect of the coding style could be one of the review comments on the patch. but I'm good with dealing with that problem when it arrives :) I'll amend those commits

This commit removes the poisoned includes as well as the exception throwing mechanism of the original class. gcc/rust/ChangeLog: * util/optional.h: Adapt for GCC.

tschwinge · 2023-06-29T20:56:26Z

gcc/rust/util/optional.h

+
+    gcc_unreachable();


Just to verify: does gcc_unreachable here (and likewise elsewhere) have the correct semantics:

gccrs/gcc/system.h

Lines 828 to 831 in 7af86ea

/* Use gcc_unreachable() to mark unreachable locations (like an

unreachable default case of a switch. Do not use gcc_assert(0). */

#if (GCC_VERSION >= 4005) && !ENABLE_ASSERT_CHECKING

#define gcc_unreachable() __builtin_unreachable ()

..., that is: an unreachable code path (..., and the compiler should optimized based on that knowledge), or should the original throw rather map onto an abort:

gccrs/gcc/system.h

Lines 789 to 795 in 7af86ea

/* Redefine 'abort' to report an internal error w/o coredump, and

reporting the location of the error in the source file.

Instead of directly calling 'abort' or 'fancy_abort', GCC code

should normally call 'internal_error' with a specific message. */

extern void fancy_abort (const char *, int, const char *)

ATTRIBUTE_NORETURN ATTRIBUTE_COLD;

#define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)

..., or thus internal_error?

ah, I know I have used it in other places not as an optimization unreachable but more as a "should not reach here unreachable", so under the impression that it would always do the fancy_abort. I guess we could have a rust_unreachable, at least for now, because we're nowhere close to caring about optimization and always want those fancy aborts

@tschwinge I've created a branch so the changes are made. I've added it to tomorrow's community call because I want to discuss it with you. I think there is value in having a macro that always fancy_aborts and in one that is used for optimization only in releases, but I don't think we are there yet

In any way, these changes aren't specific to this PR, so for now I think tl::optional should stay similar to the rest of the gccrs codebase and use gcc_unreachable (even if I now think it is "invalid" and should be replaced by fancy_abort). I will fix it once we've reached a decision on the matter tomorrow :)

CohenArthur added cleanup enhancement labels Jun 26, 2023

CohenArthur added this to In progress in Project Pineapple via automation Jun 26, 2023

CohenArthur mentioned this pull request Jun 26, 2023

Remove old uses of Optional<T> #2328

Merged

philberty approved these changes Jun 27, 2023

View reviewed changes

Project Pineapple automation moved this from In progress to Reviewer approved Jun 27, 2023

philberty reviewed Jun 27, 2023

View reviewed changes

philberty assigned CohenArthur Jun 27, 2023

CohenArthur force-pushed the optional-type branch from cecabf4 to f93ea89 Compare June 28, 2023 11:19

CohenArthur mentioned this pull request Jun 28, 2023

nr2.0: Add basic Rib class #2343

Merged

CohenArthur mentioned this pull request Jun 28, 2023

nr2.0: Add ForeverStack data structure. #2344

Merged

CohenArthur force-pushed the optional-type branch 2 times, most recently from fdd984f to c5beebf Compare June 29, 2023 09:53

optional: Adapt class to GCC's standards.

c5beebf

This commit removes the poisoned includes as well as the exception throwing mechanism of the original class. gcc/rust/ChangeLog: * util/optional.h: Adapt for GCC.

tschwinge reviewed Jun 29, 2023

View reviewed changes

CohenArthur added this pull request to the merge queue Jul 9, 2023

Merged via the queue into Rust-GCC:master with commit e74cb82 Jul 9, 2023
9 checks passed

Project Pineapple automation moved this from Reviewer approved to Done Jul 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optional: Import tl's optional type #2327

optional: Import tl's optional type #2327

CohenArthur commented Jun 26, 2023

tschwinge commented Jun 26, 2023

philberty left a comment

philberty Jun 27, 2023

philberty Jun 27, 2023

dkm Jun 27, 2023

CohenArthur Jun 27, 2023

tschwinge commented Jun 28, 2023

CohenArthur commented Jun 28, 2023

tschwinge commented Jun 28, 2023

CohenArthur commented Jun 29, 2023

tschwinge commented Jun 29, 2023

CohenArthur commented Jun 29, 2023

tschwinge Jun 29, 2023

CohenArthur Jul 9, 2023

CohenArthur Jul 9, 2023

CohenArthur Jul 9, 2023

	/* Use gcc_unreachable() to mark unreachable locations (like an
	unreachable default case of a switch. Do not use gcc_assert(0). */
	#if (GCC_VERSION >= 4005) && !ENABLE_ASSERT_CHECKING
	#define gcc_unreachable() __builtin_unreachable ()

	/* Redefine 'abort' to report an internal error w/o coredump, and
	reporting the location of the error in the source file.
	Instead of directly calling 'abort' or 'fancy_abort', GCC code
	should normally call 'internal_error' with a specific message. */
	extern void fancy_abort (const char , int, const char )
	ATTRIBUTE_NORETURN ATTRIBUTE_COLD;
	#define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)

optional: Import tl's optional type #2327

optional: Import tl's optional type #2327

Conversation

CohenArthur commented Jun 26, 2023

tschwinge commented Jun 26, 2023

philberty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschwinge commented Jun 28, 2023

CohenArthur commented Jun 28, 2023

tschwinge commented Jun 28, 2023

CohenArthur commented Jun 29, 2023

tschwinge commented Jun 29, 2023

CohenArthur commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment