Skip to content
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

sycl::buffer::set_final_data is tested with std::shared_ptr which is not required by the spec #877

Open
nilsfriess opened this issue Feb 28, 2024 · 3 comments

Comments

@nilsfriess
Copy link
Contributor

In the spec, the templated type Destination of the parameter finalData that is passed to sycl::buffer::set_final_data is described as:

Destination can be either an output iterator or a std::weak_ptr.

In the buffer tests (specifically in this file, lines 66 and 69), set_final_data is also tested with Destination == std::weak_ptr<T[]> and Destination == std::shared_ptr<T[]>, and I don't think that these types are required as per the spec.

So I think either the spec has to be extended or these test cases should be removed from the CTS (and a test case for Destination == std::weak_ptr<T> should be added).

@gmlueck
Copy link
Contributor

gmlueck commented Feb 28, 2024

I agree that the spec is not clear and that there is a disconnect with the test suite. Looking at the DPC++ implementation, it seems that we allow Destination to be any type that is convertible to std::weak_ptr, though the spec doesn't say that. This is why DPC++ accepts std::shared_ptr.

The spec is also not clear about what type of std::weak_ptr is allowed for Destination. Must it be std::weak_ptr<T>, or can it be std::weak_ptr<T[]>? What about std::weak_ptr<U>, where T is convertible to U?

We might also want to tighten up the specification to say that the output iterator must allow writing objects of type T.

@keryell
Copy link
Member

keryell commented Feb 29, 2024

I think this is because at some point in the SYCL 1.2 specification there was some special handling for std::shared_ptr but also std::shared_ptr in some other places. Then I realized that they were redundant because of some implicit conversions.

@gmlueck
Copy link
Contributor

gmlueck commented Feb 29, 2024

Implicit conversions happen only after template selection, though. The problem is that set_final_data is defined like:

template <typename Destination = std::nullptr_t>
void set_final_data(Destination finalData = nullptr);

Constraints: Available only when Destination is an output iterator or is std::weak_ptr<T>.

If the user calls set_final_data with std::shared_ptr<T>, then the template doesn't add any candidates to the overload set because Destination doesn't satisfy the constraints. If we want to allow std::shared_ptr<T> as an argument type, we need to add it somehow to the constraints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants