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

Fix issues #47

Merged
merged 39 commits into from
Aug 21, 2024
Merged

Fix issues #47

merged 39 commits into from
Aug 21, 2024

Conversation

steve-downey
Copy link
Member

No description provided.

Restore Makefile to run compile matrix
Add gcc 15 (trunk) toolchain
New release (!) of googletest.
Docs still suggest that main should be tracked.
Need to set which config to run tests with.
Remove reserved names, avoid trailing underscores in various parameter names.
Make tests pass.
   use construct_at
Force immediate function evaluation.
consteval functions to test constexpr-ness.
Add test for emplace with initializer_list
Add constexpr to swap and emplace
Fix test coverage, fix typo "addressof(&value_)", consteval tests.
Make assignment operators constexpr
missing const on one overload (unlike the other monadic ops)
missing constexpr on && overload
Add tests to cover.
Change to static_assert and remove_cv_t. See issue bemanproject#38
Use static assert to mandate T not nullopt_t
Change or add static_asserts to correspond to Mandates clauses. `requires` is
the wrong mechanism.

Also weaken the constraint on `transform` as `is_object_v` rules out a U&,
however optional<U&> is allowed in beman::optional.

Note to follow up change in paper wording.
Apply 'Mandates' as static_asserts
static in headers is bad. Subtle ODR violations as the variable is distinct.
In order to stand-alone, Beman optional does not require <optional> and uses
its own in_place_t and nullopt_t. Possibly revist, but be consistent.
@steve-downey
Copy link
Member Author

Fixes in the primary template for issues #36 #37 #38 #29 #40 #41 #42 #43 #44

Plan to add fix for #45 also.

Need to review the reference specialization for similar issues.

Increase efficiency, reduce complexity. Suggestion from @jwakely in bemanproject#45
Change is_optional to a variable template
These are "Working on my machine"
@steve-downey steve-downey marked this pull request as ready for review July 22, 2024 03:32
@steve-downey steve-downey requested a review from JeffGarland July 22, 2024 03:33
steve-downey and others added 8 commits July 27, 2024 19:35
Use test_types for no default etc.
use std::ignore for unused variables.
Fix warning about non_trivial_copy_assignable default copy constructor
unqualified move in yield_if
Consteval identity function requires contexpr parameter. Cleaner and shorter
code.
The function must return a type that an optional can hold, so either an object
or a reference.
Fix assignment operator for assign from optional<U> where U is something that
can be bound to a T&. Delete the assignment from an rvalue ref as there is
nothing to move and taking the pointer to temp will dangle.
Add test_tupes.cpp to make sure test_types.hpp stands alone and has a home in
the compilation db.
Fixes bemanproject#49 for the constructor case. Adds failing tests, and fixes them.
Copy link
Member

@neatudarius neatudarius left a comment

Choose a reason for hiding this comment

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

LGTM overall, check my current feedback:

steve-downey and others added 3 commits August 12, 2024 19:40
This avoids the most common dangling issue. Commented out test and todo
complete.
Right justify mode selector in optional.cpp
Add block to test_types.cpp
Copy link
Member

@neatudarius neatudarius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@steve-downey steve-downey merged commit af67bc6 into bemanproject:main Aug 21, 2024
7 checks passed
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.

2 participants