-
Notifications
You must be signed in to change notification settings - Fork 251
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
Rework of is
that adds new functionalities or simplify implementation
#701
Conversation
See #575 (comment):
|
Thank you. @hsutter propose the syntax on cpp2 side. I have prepared the implementation on C++20 side, and the intention is to be able to use it e.g. in inspect x -> std::string {
is <std::integral> = "x is integral type";
is <std::floating_point> = "x is floating point type";
is _ = "other type";
} |
If |
I don't know. I know that on cppfront, we don't know if something is a concept or a type (at least, I did not find a way to distinguish that on the C++ side). e.g. in |
Yeah, there's no way. |
In the cases you mentioned, I guess that we don't need additional |
I think I understand now. |
I was interested in making C++ code work. Based on that I was able to call it from cpp2. |
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.
This is preexisting, but I have previously commented at https://github.com/hsutter/cppfront/pull/529/files/06d8e5f1229454b6b1a1a392be00d887b03d5e0c#diff-9b9f6d5ded63da9a82c7f23fde117cd494a355abad3624569e71a7bf73f974deR81-R82:
// Like in P2392, the cases of the built-in `is` should be a chain of conditions.
// Using overloads to implement that is tedious and error-prone.
I think that has more potential for a simpler implementation.
I wouldn't be surprised if the use of non
-concepts or negative constraints dropped to zero.
include/cpp2util.h
Outdated
@@ -1053,15 +1229,16 @@ inline constexpr auto program_violates_type_safety_guarantee = sizeof...(Ts) < 0 | |||
|
|||
// For literals we can check for safe 'narrowing' at a compile time (e.g., 1 as std::size_t) | |||
template< typename C, auto x > | |||
inline constexpr bool is_castable_v = | |||
concept castable = requires{ C{x}; } || ( | |||
std::is_integral_v<C> && | |||
std::is_integral_v<CPP2_TYPEOF(x)> && | |||
!(static_cast<CPP2_TYPEOF(x)>(static_cast<C>(x)) != x || | |||
( | |||
(std::is_signed_v<C> != std::is_signed_v<CPP2_TYPEOF(x)>) && |
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.
(std::is_signed_v<C> != std::is_signed_v<CPP2_TYPEOF(x)>) && | |
different_sign_types<C, CPP2_TYPEOF(x)> && |
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.
Oh, yes... I was reminding myself to correct this line.
Yeah, the functionality works fine. |
I found errors in my use of concepts (in terms of benefit from subsumption rules). I will update it soon. |
58d6ac7
to
f10b9fc
Compare
Although it's in a different context, |
Cool! I will take a look at that. Meanwhile, I am working on correcting this PR, as I found some things that could be improved... |
4dfd8df
to
f82a67b
Compare
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.
This goes a long way to addressing #492.
It falls short due to:
Once
operator is
is constrained or answered statically,
it becomes necessary to fixinspect
to omit alternatives whose conditions are non-viable.
Which doesn't seem to be a problem here yet.
That may be due to is
overloads like for std::variant
,
which still returns bool
even though sometimes it's always false
.
I argue that, ideally, that'd be constrained so as
to get a compile-time error outside inspect
.
include/cpp2util.h
Outdated
// and "as std::string" for the same cases | ||
// | ||
template <typename T> | ||
concept has_to_string_overload = requires{ static_cast<std::string (*)(T const&)>(&to_string); }; |
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.
There's this overload:
inline auto to_string(std::string const& s) -> std::string const&
{
return s;
}
I don't think std::string
satisfies has_to_string_overload
.
Anyways, the fact that it has a different return type has always tingled my spider senses.
I've always thought it'd be easy to inadvertently get a dangling reference in a generic context.
There's also these overloads:
inline auto to_string(bool b) -> std::string
{
return b ? "true" : "false";
}
inline auto to_string(char const* s) -> std::string
{
return std::string{s};
}
Those would also probably fail due to the argument not being a const&
.
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 will check these... all my tests pass. Nevertheless, I will double-check these.
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.
It seems like only the last one doesn't pass: https://cpp2.godbolt.org/z/871q7P5W7.
I wonder why the others do.
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.
cpp2::has_to_string_overload<bool>
probably passes thanks to
template<typename T>
inline auto to_string(T const& t) -> std::string
requires requires { std::to_string(t); }
{
return std::to_string(t);
}
As that's valid: https://cpp2.godbolt.org/z/4vT5dPcP9.
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.
cpp2::has_to_string_overload<std::string>
passes thanks to
template<typename T>
inline auto to_string(T const& sv) -> std::string
requires (std::is_convertible_v<T, std::string_view>
&& !std::is_convertible_v<T, const char*>)
{
return std::string{sv};
}
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.
It makes no sense.
Actually calling it chooses the overload returning std::string const&
:
https://cpp2.godbolt.org/z/YzPf9ovjv.
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.
You were right. I made a mistake here. I put the corrected version here: https://godbolt.org/z/WbGqGnEY1
the const char*
was matching the as(bool x)
and there were more issues... I believe I have solved them... I will run the tests after Bjarne's talk.
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 believe I have fixed that.
426e833
to
73a1983
Compare
@@ -13,7 +13,7 @@ class Square : public Shape { }; | |||
print: ( msg: std::string, x: _ ) = | |||
std::cout << msg << x << "\n"; | |||
|
|||
print: ( msg: std::string, b: bool ) = | |||
print: <B:std::convertible_to<bool>> ( msg: std::string, copy b: B ) = |
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.
FYI, what I did at https://github.com/hsutter/cppfront/pull/529/files#diff-508db04ed7e2433d79b037f5473ae5ca5330b4b04b842136ad0c95a40142fee8
was to remove this function and preface main
with std::cout << std::boolalpha;
.
A more recent alternative to std::cout << std::boolalpha;
might be to change x
above to x as std::string
.
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 just thought that two versions of the print
functions were to show that the result will match the one with bool
. If it is not needed then your version is the right one.
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.
We'd have to ask @hsutter for the purpose.
73a1983
to
09e08ea
Compare
348ecd1
to
13137b4
Compare
I have reworked a lot during my preparation for the talk on cppcon. After the talk, I have done even more... I am changing this PR to the Draft for a moment as I will clean it up. The current status is that it can handle many more cases and switches to perfect forwarding. I will be traveling today, so I may manage to clean it up. |
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.
This many overloads, constraints, and negative constrains, really is nightmare fuel.
I really think we should tame these overloads with more if constexpr
instead of overloads.
For example, rather than one overload with first template parameter std::same_as<std::string>
for each kind of function argument,
e.g.,
std::same_as<std::any> const&
typename X>
requires has_to_string_overload<std::remove_cvref_t<X>>
&& (!std::same_as<std::remove_cvref_t<X>, std::any>)
&& (!std::same_as<std::remove_cvref_t<X>, std::string>)
auto as( X&& x )
typename X>
requires pointer_like<std::remove_cvref_t<X>>
auto as( X&& x )
typename X>
requires pointer_like<std::remove_cvref_t<X>>
&& std::same_as<typename X::element_type, std::string>
auto as( X&& x )
typename X>
requires (!has_to_string_overload<std::remove_cvref_t<X>>)
&& (!std::same_as<std::remove_cvref_t<X>, std::string>)
&& (!pointer_like<std::remove_cvref_t<X>>)
auto as( X&& )
have one single overload with first template parameter std::same_as<std::string>
,
and factor in the overloads' function parameters, like
template <std::same_as<std::string> C, typename X>
auto as( X&& x ) -> decltype(auto) {
// No `std::same_as<std::any> const&`,
// instead rewrite is as `cpp2::op_as` to further tame the big `as` overload.
if constexpr (requires { op_as<C>(std::forward<X>(x)); }) {
return op_as<C>(std::forward<X>(x)); // `op_as` for `::std` types need to before these `as` overloads.
}
else if constexpr (has_to_string_overload<std::remove_cvref_t<X>>
&& (!std::same_as<std::remove_cvref_t<X>, std::any>)
&& (!std::same_as<std::remove_cvref_t<X>, std::string>)) {
return to_string(x);
}
else if constexpr (pointer_like<std::remove_cvref_t<X>>) {
if (x) {
return as<C>(forward_like<X>(*x));
}
return std::string("(empty)");
}
else if constexpr (pointer_like<std::remove_cvref_t<X>>
&& std::same_as<typename X::element_type, std::string>) {
if (x) {
return as<C>(forward_like<X>(*x));
}
return std::string("(empty)");
}
else if constexpr ((!has_to_string_overload<std::remove_cvref_t<X>>)
&& (!std::same_as<std::remove_cvref_t<X>, std::string>)
&& (!pointer_like<std::remove_cvref_t<X>>)) {
return nonesuch_<casting_errors::no_to_string_cast>{};
}
}
Then it becomes much easier
to see what's going on,
to simplify by removing negative requirements handled by earlier branches,
and possibly more good stuff.
include/cpp2util.h
Outdated
constexpr inline nonesuch_<> nonesuch; | ||
|
||
template <typename X> | ||
concept nonesuch_specialization = requires (std::remove_cvref_t<X> x) { |
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.
concept nonesuch_specialization = requires (std::remove_cvref_t<X> x) { | |
concept nonesuch_specialization = requires (X x) { |
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 will check that... I have started to put std::remove_cvref_t<X>
in a lot of the places as I was switching to perfect forwarding... and all type traits stop to work.
include/cpp2util.h
Outdated
} | ||
|
||
template <typename T> | ||
using pointee_t = std::remove_cvref_t<decltype(*std::declval<std::remove_cvref_t<T>>())>; |
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.
Seems like you want std::iter_value_t<T>
.
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.
pointee_t
is also used for things like std::optional
, smart pointers, etc... in general, anything you can dereference.
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.
It works with those: https://compiler-explorer.com/z/af1c11aa5.
include/cpp2util.h
Outdated
concept specialization_of_template = requires (std::remove_cvref_t<X> x) { | ||
{ is<C>(std::forward<X>(x)) } -> std::same_as<std::true_type>; |
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.
concept specialization_of_template = requires (std::remove_cvref_t<X> x) { | |
{ is<C>(std::forward<X>(x)) } -> std::same_as<std::true_type>; | |
concept specialization_of_template = requires { | |
{ is<C>(std::declval<X>()) } -> std::same_as<std::true_type>; |
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.
most probably std::remove_cvref_t<X>
and std::forward<X>(x)
are not needed. I have been wondering what is better: use std::declval<X>()
or requires(X x) { ... }
. The second option is more compelling to me (especially after writing many concepts.
I am continue to clean this code up.
include/cpp2util.h
Outdated
// else | ||
return as<C>(CPP2_FORWARD(x)); | ||
program_violates_type_safety_guarantee<C, X>, | ||
"No cpp2::to_string overload exists for this type!"); |
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.
How is this violated?
There's a catch-all overload inline auto to_string(...) -> std::string
.
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.
Yes, that is right. There is a catch-all overload. That works for cpp2::to_string
but is incompatible with the behavior of as
when the cast is impossible.
The contract for as
is that when you call it, you either have the correct output or an error (static_assert
when possible and an exception on runtime).
Having as
silently return "(customize me - no cpp2::to_string overload exists for this type)"
breaks that contract. We can detect that on compile time, so I think static_assert
is the right choice.
A similar thing will happen when you try to cast int
to size_t
(size_t
cannot represent all values of int
, so you will get static_assert
with a suggestion to use unsafe_narrow<T>()
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 agree that choosing that overload is wrong.
It's good to know that using as std::string
doesn't choose it.
I guess it works this way because the concept that checks for the overload checks for conversion to function pointer, and the (...)
parameter list doesn't work with those.
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.
Yes. It does not cast.
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.
That is why I introduce this concept.
Regarding many overloads. It is my work-in-progress thing, and I want to clean it up. Having multiple overloads allows us to create methods that will return As we want to allow for the custom operator as, and operator is, I started to worry that having a |
I am preparing a lot of test cases as it is really hard to spot all those errors. I have also found some errors that were silently there but changing the approach made them errors. |
If a single branch is chosen, there's no chance for ambiguity in the return type. |
I think it's easier to add new a branch than a new overload. |
3988e31
to
4e260fa
Compare
@hsutter I am fixing |
No worries, I appreciate all the thought and care you're putting into this. |
I have fix is() cases and I am working on as() cases. |
35a70b8
to
4f15948
Compare
@hsutter I moved the implementation forward. Most of the cases are working... I see that CI checks failed on macOS-11 and macOS-latest. I just realized the CPU architecture is the main difference between my platform and the one used on the CI side. CI uses |
Sounds good -- I'm not the expert there so I'd welcome anyone to provide a PR to add that. Thanks! |
is
and as
that adds new functionalities or simplify implementationis
that adds new functionalities or simplify implementation
@hsutter, I started splitting this big PR into smaller ones. I believe this one covers the I am sorry for the late delivery... life. |
Wait a sec... sth... is wrong with this PR... fixing. |
f161425
to
b88b154
Compare
@hsutter ok, now fixed. I messed up with branches. But now it is ready. |
@hsutter @JohelEGP I am looking at change #956, which only brings replacing multiple My implementation of I am not insisting on choosing my current solution. Let's agree on one way of implementing this kind of function. I will adjust my implementation to follow what will be decided. I have been worried about having functions with multiple constexpr ifs, but maybe it is just me. Opinions? I really want to deliver it as it has not been merged for a long time, and it is troublesome to keep it in a mergeable state. |
A chain of constexpr if is worrisome indeed, but less so than overloading. The next step would be to rewrite non-built functions as For example, you add this template< typename C, can_bound_to<C> X >
requires not_same_as<X, std::any>
constexpr auto is( X && ) -> std::true_type {
return {};
} That The next one, with Only the first one can't be integrated into the chain. |
Again, thanks for this and for waiting. I've been watching to see when I should review this -- I got the impression you were making one more change with the last discussion above, but maybe I misunderstood? Sorry if I missed that it's ready, please just let me know when it's ready to review and mergeable and I'll look at it next. |
@hsutter I am closing this... next merge request ongoing. |
This change reworks a big part of the code for
is
.type_find_if
to iterate overvariant
types to simplifyis
forvariant
,is
overloads to useconcepts
instead oftype_traits
(to utilize concepts subsumption rules),operator_is
, implementation ofis
for variants now usetype_find_if
,is
that is not using argumentx
for inspection returnsstd::true_type
orstd::false_type
Closes #689
Closes #669
Cases handled by the new
is()
:Tests
Issue
macos-11, clang++, c++20
(one of the CI targets)The original PR contained a rework of
as()
andto_string()
- they will be provided in separate PRs.