Allow passing a non-trivially-copyable comparator to sort

[dmitriy-sobolev]

Fixes #1924.

[danhoeflinger]

LGTM, this should fix the issue.
Thanks

We really need to push for the change to SYCL to make this sort of type intrinsically device copyable. Its a really frustrating maintenance problem.

Edit: added a comment which should be addressed before merging, in my opinion.

[danhoeflinger]

I'm not sure if this actually causes any problems for the compilation of the test, but we should probably create a utility range_device_copyable and a range_non_device_copyable which implement operator[], size(), begin(), end(), and provide a value and difference type in test/support/utils_device_copyable.h.
Use those types here instead of the noop functors.

[dmitriy-sobolev]

It successfully compiles with the current approach. Let me investigate why, what I should have done initially.

If it is proven to be reliable, we can avoid adding extra code in the test. If I cannot prove it, I will reuse an existing range type (if possible), and add divice_copyable/non_device_copyable comparators (it is easier).

[danhoeflinger]

I think its not fully instantiating the type in the compile time check, so it doesn't run into the issues in the type if it were created with those types. https://godbolt.org/z/d87x6eo1s

I'm a bit torn, as we could probably delete all but a simple device_copyable_type and non_device_copyable_type in our tests, but we wouldn't be checking types which are actually able to be instantiated.

I guess I'm OK with either direction we want to go. So far, we have created types to match, but wouldnt push hard against cleaning this up and relying only on a couple simple types if it works and compiles reliably for our testing purposes on our full compiler matrix.

[dmitriy-sobolev]

Thank you very much for the example and description. I have a more tailored example here: https://godbolt.org/z/3WqGT68x6. Indeed, it does not instantiate the type entirely.

It seems there is no need for the compiler to instantiate the tested class entirely in that expression, so we can get away with noop classes. The standard (c++17) says the following about it:

Unless a class template specialization has been explicitly instantiated (17.8.2) or explicitly specialized (17.8.3),
the class template specialization is implicitly instantiated when the specialization is referenced in a context
that requires a completely-defined object type or when the completeness of the class type affects the semantics
of the program.

In my understanding the note implies that a class must be instantiated when it is needed in that particular context.

[danhoeflinger]

Yeah, thanks for finding the relevant section in the specification. I think technically we will generally be OK with just the simple noop classes.

I'm still undecided which I prefer really. I think the more realized types are a bit better to document what we are doing (I always look at tests documentation, at least in part). This is a big stretch here because we are really testing internals in an obscure and specific fashion. However, it may be more confusing to future readers with less realistic non-instantiable types.

I'm OK with merging as it is if that is what you would like to do. If we want to clean up the types to remove all but a pair of simple types in a separate PR, we can do that. We can probably add some comment explaining why this works and the goal of these specific tests, if it is not already clear.

found something which needs addressing