Skip to content

Commit

Permalink
refactor: initial CPOs for representation types and their usage in re…
Browse files Browse the repository at this point in the history
…presentation concepts
  • Loading branch information
mpusz committed Nov 21, 2024
1 parent 0c09008 commit f84ed8f
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 6 deletions.
106 changes: 106 additions & 0 deletions src/core/include/mp-units/framework/customization_points.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,112 @@ constexpr bool is_vector = false;
template<typename Rep>
constexpr bool is_tensor = false;

MP_UNITS_EXPORT_END

namespace detail::inline norm_impl {

void norm() = delete; // poison pill

struct norm_t {
template<typename T>
[[nodiscard]] constexpr auto operator()(const T& vec) const
{
if constexpr (requires { vec.norm(); })
return vec.norm();
else if constexpr (requires { norm(vec); })
return norm(vec);
else if constexpr (requires { vec.magnitude(); })
return vec.magnitude();
else if constexpr (requires { magnitude(vec); })
return magnitude(vec);
// TODO Is it a good idea to enable fundamental types to represent vector quantities?
// else if constexpr (is_scalar<T>)
// return std::abs(vec);
}
};

} // namespace detail::inline norm_impl

inline namespace cpo {

MP_UNITS_EXPORT inline constexpr ::mp_units::detail::norm_impl::norm_t norm;

}

namespace detail::inline real_impl {

void real() = delete; // poison pill

struct real_t {
template<typename T>
[[nodiscard]] constexpr auto operator()(const T& clx) const
{
if constexpr (requires { clx.real(); })
return clx.real();
else if constexpr (requires { real(clx); })
return real(clx);
}
};

} // namespace detail::inline real_impl

inline namespace cpo {

MP_UNITS_EXPORT inline constexpr ::mp_units::detail::real_impl::real_t real;

}

namespace detail::inline imag_impl {

void imag() = delete; // poison pill

struct imag_t {
template<typename T>
[[nodiscard]] constexpr auto operator()(const T& clx) const
{
if constexpr (requires { clx.imag(); })
return clx.imag();
else if constexpr (requires { imag(clx); })
return imag(clx);
}
};

} // namespace detail::inline imag_impl

inline namespace cpo {

MP_UNITS_EXPORT inline constexpr ::mp_units::detail::imag_impl::imag_t imag;

}

namespace detail::inline modulus_impl {

void modulus() = delete; // poison pill

struct modulus_t {
template<typename T>
[[nodiscard]] constexpr auto operator()(const T& clx) const
{
if constexpr (requires { clx.modulus(); })
return clx.modulus();
else if constexpr (requires { modulus(clx); })
return modulus(clx);
// `std` made a precedence of using `abs` for modulo on `std::complex`
else if constexpr (requires { abs(clx); })
return abs(clx);
}
};

} // namespace detail::inline modulus_impl

inline namespace cpo {

MP_UNITS_EXPORT inline constexpr ::mp_units::detail::modulus_impl::modulus_t modulus;

}

MP_UNITS_EXPORT_BEGIN

/**
* @brief A type trait that defines zero, one, min, and max for a representation type
*
Expand Down
10 changes: 4 additions & 6 deletions src/core/include/mp-units/framework/representation_concepts.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,9 @@ concept ComplexRepresentation = Complex<T> && WeaklyRegular<T> && requires(T a,
{ a - b } -> Complex;
{ a* b } -> Complex;
{ a / b } -> Complex;
{ real(a) } -> Scalar;
{ imag(a) } -> Scalar;
{ abs(a) } -> Scalar;
{ arg(a) } -> Scalar;
{ conj(a) } -> Complex;
{ ::mp_units::real(a) } -> Scalar;
{ ::mp_units::imag(a) } -> Scalar;
{ ::mp_units::modulus(a) } -> Scalar;
};

// TODO how to check for a complex(Scalar, Scalar) -> Complex?
Expand All @@ -138,8 +136,8 @@ concept VectorRepresentation = Vector<T> && WeaklyRegular<T> && requires(T a, T
{ -a } -> Vector;
{ a + b } -> Vector;
{ a - b } -> Vector;
{ ::mp_units::norm(a) } -> Scalar;
// TBD
// { norm(a) } -> Scalar;
// { zero_vector<T>() } -> Vector;
// { unit_vector(a) } -> Vector;
// { scalar_product(a, b) } -> Scalar;
Expand Down

18 comments on commit f84ed8f

@JohelEGP
Copy link
Collaborator

Choose a reason for hiding this comment

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

It has come to my attention that the representation concepts do not requrie the implicit expression variations.
That's because the requires's parameters have type T and not const T&.

@JohelEGP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, the CPOs seem to be lacking https://eel.is/c++draft/customization.point.object#6,
which instead are in the concepts.

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Nov 24, 2024

Choose a reason for hiding this comment

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

I am not sure what you mean here.

If concepts are defined in terms of CPOs, then CPOs probably can't use those concepts to constrain the return type. It is like trying to constrain std::ranges::swap arguments with std::swappable.

Can you provide some examples or submit a PR with the changes you would like to see?

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Nov 24, 2024

Choose a reason for hiding this comment

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

Please note that the intent is to remove is_scalar and detail::Scalar & Co. As you suggested some time ago, if it is only possible, we should remove those and replace them with just syntactic checks on a type.

@JohelEGP
Copy link
Collaborator

Choose a reason for hiding this comment

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

std::ranges::range is defined as (https://eel.is/c++draft/range.range):

template<class T>
  concept range =
    requires(T& t) {
      ranges::begin(t);         // sometimes equality-preserving (see below)
      ranges::end(t);
    };

It has the requirement ranges::begin(t);
rather than { ranges::begin(t) } -> input_or_output_range;.
ranges::begin itself requires its result to be an input_or_output_range.

@JohelEGP
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is like trying to constrain std::ranges::swap arguments with std::swappable.

4
#
[Note 5: The semantics of the swappable and swappable_with concepts are fully defined by the ranges​::​swap customization point object.
— end note]

@JohelEGP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these poison pills need to be specified like
https://eel.is/c++draft/concept.swappable#2.1 or
https://eel.is/c++draft/range.access.begin#2.5?

Also, for these to be CPOs, they need to be constrained to meet https://eel.is/c++draft/customization.point.object#4.sentence-2.

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Nov 24, 2024

Choose a reason for hiding this comment

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

It has the requirement ranges::begin(t);
rather than { ranges::begin(t) } -> input_or_output_range;.
ranges::begin itself requires its result to be an input_or_output_range.

Sure, but begin() is not constrained with range.

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Nov 24, 2024

Choose a reason for hiding this comment

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

Also, for these to be CPOs, they need to be constrained to meet https://eel.is/c++draft/customization.point.object#4.sentence-2.

My intent was to constrain concepts with CPO invocation (like swappable). In this case, the argument and return type can't be constrained by the same concept. If something supports norm it is a VectorRepresentation. If something supports re and im, it is ComplexRepresentation. I do not know how to constrain the input with the same concept as well. Maybe we can constrain the output with ScalarRepresentation in such a case, but it means that we will have to split and intermix all the concepts and customization points definitions, which I am not sure if I like.

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Nov 24, 2024

Choose a reason for hiding this comment

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

If you know how to resolve the above cyclic constraints, please share a PR. Please note, that what we have is still not the final version as we still depend on is_xxx customization points. We will eventually remove those, but to do that, I need to implement proper handling of quantity specs for various representations.

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Nov 24, 2024

Choose a reason for hiding this comment

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

Also, for these to be CPOs, they need to be constrained to meet https://eel.is/c++draft/customization.point.object#4.sentence-2.

Please note that all return auto. For unsupported cases, nothing is returned, which means that the return type can't be deduced.

@JohelEGP
Copy link
Collaborator

@JohelEGP JohelEGP commented on f84ed8f Nov 24, 2024

Choose a reason for hiding this comment

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

I don't see any cycle, just like the C++ Standard CPOs don't have any.

Please note that all return auto. For unsupported cases, nothing is returned, which means that the return type can't be deduced.

It deduces to void,
and doesn't meet the requirements of the CPO
(e.g., for mp_units::real to return the real part).

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Nov 24, 2024

Choose a reason for hiding this comment

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

Do these poison pills need to be specified like
https://eel.is/c++draft/concept.swappable#2.1 or
https://eel.is/c++draft/range.access.begin#2.5?

I am not sure. The idea is to force the name lookup to only use ADL and not scope resolution. We do not want to look for overloads in the parent scopes and the global scope.

@JohelEGP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget about f84ed8f#commitcomment-149469198.

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Nov 26, 2024

Choose a reason for hiding this comment

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

I am not sure what you meant in this comment. Can you provide some example?

@JohelEGP
Copy link
Collaborator

@JohelEGP JohelEGP commented on f84ed8f Nov 26, 2024

Choose a reason for hiding this comment

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

concept Scalar = (!disable_scalar<T>) && WeaklyRegular<T> && requires(T a, T b) {
// scalar operations
{ -a } -> std::common_with<T>;

This -a requires a modifiable lvalue a.
-T{} is not required, or any other combination that is not a non-const lvalue.
If the parameter was const T& a instead of T a,
the implicit expression variations would make it as if
-std::move(a),
-b, and
-std::move(b)
were also required with the same semantics,
where b is declared T b or T& b.

@mpusz
Copy link
Owner Author

@mpusz mpusz commented on f84ed8f Dec 1, 2024

Choose a reason for hiding this comment

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

Please let me know if a444c53 is what you meant.

@JohelEGP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that looks better.

Please sign in to comment.