-
Notifications
You must be signed in to change notification settings - Fork 68
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
Numeric ranges #312
Open
cjdb
wants to merge
9
commits into
CaseyCarter:master
Choose a base branch
from
cjdb:numeric-ranges
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Numeric ranges #312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjdb
commented
Jun 23, 2019
cjdb
commented
Jun 23, 2019
namespace ext { | ||
template<class T, class U> | ||
META_CONCEPT __differenceable_with = __summable_with<T, U> && | ||
requires(__uncvref<T> t, __uncvref<U> u) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that T
and U
have had their cv- and ref-qualifiers removed, but I can't think of a better way to implement this at the moment.
cjdb
commented
Jun 23, 2019
This commit does all the necessary groundwork to get a basis for accumulate to be a ranges citizen. We achieve this by introducing: * the concept Magma * the concept IndirectMagma * a new set of arithmetic operation function objects * an accumulate niebloid * accumulate tests == Notes std::multiplies and std::divides have been renamed to `product` and `quotient`, respectively. Each arithmetic operation has its own detail concept that effectively check things like "has plus". Care was taken to ensure that the requirements for each operator were as complete as possible. For example, since subtraction is the inverse of addition, anything that supports ranges::ext::minus should also support ranges::ext::plus. == Known issues It's believed that each of the detail concepts are ill-specified with respect to references. For example, most expressions are checked à la `{ std::forward<T>(t) + std::forward<T>(t) } -> Common<T>;`, but the type of `t` is `__uncvref(t)` due to difficulties getting some expressions to compile. Reviewers should note this problem when evaluating the design of the detail concepts.
…oads == change-log * shuffles around entities to remove circular dependencies introduced by this commit. * adds identity numeric traits * adds identities for all arithmetic operations * adds Semigroup concept * adds Monoid concept * adds IndirectSemigroup concept * adds IndirectMonoid concept * adds monoid overload subset for accumulate == left_identity, right_identity, and two_sided_identity The library needs to support two-sided identities in order to facilitate monoids and loops. Similarly to common_type, a user is allowed to specialise both left_identity and right_identity (they are in fact required to do so for an identity to exist). A user may not specialise the two_sided_identity template. A well-formed left_identity and right_identity specialisation has an explicit constructor taking a binary operation, a T, and a U. The constructor should also require that the binary operation is a magma over T and U. The identity specialisations also have a const-qualified value member function that returns the value of the identity. A well-formed two_sided_identity is only possible if the left_identity and the right_identity return the same type and value. All the arithmeric operations except for modulus added in ac25864 have been given either a left_identiy or a right_identity, but only plus and product have both. === Known issues left_identity and right_identity currently permit specialisations to also take a reference-to-const T or U. It is likely that this will cause lifetime issues, and the author is considering changing this from a reference to an object. == accumulate overload subset for monoid operations over T and U std::accumulate requires -- as input -- an initial value to determine the return type of the function. This was somewhat mitigated by the introduction of std::reduce, which has an overload only taking an iterator-pair. This is not a complete solution, as that overload is equivalent to calling the overload taking iter_value_t<I>{} and std::plus{}. In other words, `reduce(first, last)` is only possible if your reduction is an addition operation. The standard library prevents other operations from working because different operations treat values differently. For example, anything multiplied by zero is also zero, and it would have been flawed for the specification to permit this. The only safe option was to have the interface be a convenience for addition. The niebloid-equivalent accumulate has been designed so that there are two overload subsets: the magma overload subset and the monoid overload subset. The magma overload subset is the rangified equivalent of std::accumulate, requires only that the binary operation models a Magma over T and U, and was commit in ac25864. The monoid overload subset refines the requirements by requiring the binary operation model a Monoid over T and U. Because a monoid requires that the operation have a two-sided identity, we're able to provide a pair of overloads that don't need an initial value for any operation: a user can simply specialise the left_identity and right_identity numeric-traits, and accumulate will take care of the rest. === Known issues The decision for this overload subset to require a monoid instead of just a left_identity is because accumulate perfoms a left-fold operation, and C++ does not have a right-fold operation. To get a right-fold, one must iterate over the range in reverse, and so requiring only a left_identity is too weak. A monoid is perhaps over-constraining, as the monoid subset will reject a subtractive fold, even though associativity isn't required. It's unclear to the author how to proceed in this manner.
…espace The ranges-equivalent of std::negate was forgotten in the previous commit. This commit is to rectify that mistake. The requirement for a type to have a unary operator- is now under the jurisdiction of the implementation-detail concept __negatable, which is refined by __differenceable_with. A slight bug was identified: -t should not be required to return the same type as T, but rather a type in common with T. Finally, there were a few whitespace issues that were noticed. They've been cleaned up.
The design of this algorithm is slightly different to the one found in range-v3, as it has a slightly weaker requirement. The range-v3 cousin requires the binary operation to be a semigroup over the projection of I; but the author couldn't see a reason for partial_sum to require associativity. As such, the implementation here only requires that the binary operation model a magma over the projection of I.
The given modulus right-identity was not actually an identity element.
This is a direct port of std::adjacent_difference, in the image of ranges::partial_sum.
This is probably some of the most questionable code that I've written, but is necessary because C++20 doesn't support concept templates. The test attached demonstrates that Magma, Semigroup, and Monoid work as expected for lvalues, rvalues, and references. It's likely incomplete. The tests identified that there were a few errors embedded into the design of `two_sided_identity`, and promted a small patch to some code that GCC permits, but the IS does not.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an attempt at getting the design for numeric ranges going (third time lucky?). There's a lot of design documentation in the commit messages; and possibly some rambling too.
The only algorithms this PR commits at present are:
accumulate
partial_sum