[xutility] Modernize _Ptr_meow_cat to use variable templates

[Michael Schellenberger Costa (miscco)]

This is a work in progress for #860 that I started yesterday.

With Stephan T. Lavavej (@StephanTLavavej) awesome work on _Equal_memcmp_is_safe it went quite well.

• I use an enum to differentiate between the different categories. Is that ok or should I use some other means of discriminating between the categories.

• There is some unfortunate code duplication because enable_if does not work on specializations of variable templates. (It complains about an unused template argument). Is that ok or has somebody a better idea?

• Names are hard

• That said there is some bug I need to hunt but I wanted to get the overal feedback on the approach.

• The unwrap_enum_t part is terrible I will most likelybuild a helper template for that

[Michael Schellenberger Costa (miscco)]

Casey Carter (@CaseyCarter) This passes without ranges support. The problem is again that contiguous_iterator<volatile int*> returns false, which breaks all the tests in C++20 mode.

I seem to remember that you already filed an issue regarding some bug in the ternary operator that leads to common_with to be false but was unable to find it. Has that been resolved?

[Michael Schellenberger Costa (miscco)]

I also found that iter_value_t breaks everything here. I would have thought I could use it instead of remove_cv_t<remove_reference_t<_Iter_ref_t<_Iter1>>> but ...

e.g. std::iter_value_t<std::ostreambuf_iterator<char,std::char_traits<char>>> gives me this error

error C2794: 'value_type': is not a member of any direct or indirect base class of 'std::indirectly_readable_traits<std::ostreambuf_iterator<char,std::char_traits>>'
indirectly_readable_traits<remove_cvref_t<_Ty>>, iterator_traits<remove_cvref_t<_Ty>>>::value_type;

[Michael Schellenberger Costa (miscco)]

I have trouble understanding the test errors, as those specializations they complain do work fine in the test.

Can somebody that understands meta programming help?

[Michael Schellenberger Costa (miscco)]

So it seems that the two overloads for same type copy and pointer copy were indeed ambiguous if _Source == _Dest.

I added some SFINAE or that to same type copy specializations.

There is still a question why MSVC accepts this code when then other major compilers do.

I will try to condense this down a bit and create a ticket

EDIT: filed here https://developercommunity.visualstudio.com/content/problem/1058776/compiler-accepts-ambiguous-overload-of-variable-te.html

[Michael Schellenberger Costa (miscco)]

Casey Carter (@CaseyCarter) can I get some direction on how to fix the ostreambuf mess?

[Michael Schellenberger Costa (miscco)]

I addressed most of the stylistic comments and will have a look on how to fix those failures

[Michael Schellenberger Costa (miscco)]

This is driving me crazy. Why it is trying to do sizeof(void) in _Can_memcpy_integrals when is_integral<void> should already have returned false?

[Adam Bucior (AdamBucior)]

This is driving me crazy. Why it is trying to do sizeof(void) in _Can_memcpy_integrals when is_integral<void> should already have returned false?

std::conjunction short circuiting doesn't work that way. Template parameters need to be evaluated before template is instantiated. You need:

template <class _Source, class _Dest>
struct _Find_some_name_for_this_struct
{
    static constexpr bool value = sizeof(_Source) == sizeof(_Dest) && is_same_v<bool, _Source> == is_same_v<bool, _Dest>;
};

and then:

constexpr bool _Can_memcpy_integrals = conjunction_v<is_integral<_Source>, is_integral<_Dest>,
    _Find_some_name_for_this_struct<_Source, _Dest>>;

[Michael Schellenberger Costa (miscco)]

This is driving me crazy. Why it is trying to do sizeof(void) in _Can_memcpy_integrals when is_integral<void> should already have returned false?

std::conjunction short circuiting doesn't work that way. Template parameters need to be evaluated before template is instantiated. You need:

template <class _Source, class _Dest>
struct _Find_some_name_for_this_struct
{
    static constexpr bool value = sizeof(_Source) == sizeof(_Dest) && is_same_v<bool, _Source> == is_same_v<bool, _Dest>;
};

and then:

constexpr bool _Can_memcpy_integrals = conjunction_v<is_integral<_Source>, is_integral<_Dest>,
    _Find_some_name_for_this_struct<_Source, _Dest>>;

Argh damn it, I am always forgetting that...

[Michael Schellenberger Costa (miscco)]

I am super unhappy that I had to essentially roll back the most useful changes to go back to that pointer only specialization.

However, it seems like there are some deficiencies wrt iter_reference_t and iter_value_t

[Michael Schellenberger Costa (miscco)]

I think this should be ready for the next round of reviews

[Miya Natsuhara (mnatsuhara)]

It seems like the spacing around bool_constant is different here than in other occurrences (specifically before and after the < and >), and it also looks like it is preexisting in the code before changes. Is there a reason for this?

[Michael Schellenberger Costa (miscco)]

I believe this is clang format getting confused by the _Uses_default_construct<_Alloc, _Ptrval, decltype(_STD move(*_UFirst))>::value > {} afterwards

I was not able to create a simple reproducer though

[Michael Schellenberger Costa (miscco)]

I would like to add a rename here too:
_Copy_memmove ==> _Memmove_forward
_Copy_memmove_backward ==> _Memmove_backward
_Copy_memmove_common ==> _Memmove_forward_bounded

We use these functions all over the place in copy and move so the _Copy prefix just adds confusion as to what is happening.

Any disagreement?

[Adam Bucior (AdamBucior)]

Shouldn't _Copy and _Copy_uninitialized become _Move and _Move_uninitialized with move_iterator? (I know it's pre-existing but it's definitely wrong)

[Michael Schellenberger Costa (miscco)]

I think that you are right that for the _Copy case it seems wrong. For _Copy_uninitialized we require trivial types anyway where it does not make a difference.

Thatsaid I am absolutely in favor of changing it accordingly

[Adam Bucior (AdamBucior)]

Since we fixed it in #1772 it's no longer pre-existing so we should change this accordingly.

[Stephan T. Lavavej (StephanTLavavej)]

This PR has merge conflicts now that we're using if constexpr unconditionally.

[Michael Schellenberger Costa (miscco)]

Casey Carter (@CaseyCarter) I have rebased and updated the PR. I think we should decide whether this is worth it or simply put it in the bin

[Michael Schellenberger Costa (miscco)]

The gods have failed me :(

[Stephan T. Lavavej (StephanTLavavej)]

There are a number of merge conflicts - can you resolve them?

[Michael Schellenberger Costa (miscco)]

Yeah, will do over the weekend

[Stephan T. Lavavej (StephanTLavavej)]

After merging your #1771, there's a conflict in <vector>.

[Michael Schellenberger Costa (miscco)]

So I am going to drop this and replace it with a sequence of smaller less error prone PRs:

1. Add variable templates for the type aliases and use them in the Codebase
   2.-N Refactor the implementation of each variable template into a right thing.

Does that sound like a plan?

[Adam Bucior (AdamBucior)]

Yeah, I think it's a good idea to split this into smaller PRs.