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

CmdUpdateBuffer pData parameter should be const #981

Open
jduncanator opened this issue Jul 31, 2022 · 3 comments
Open

CmdUpdateBuffer pData parameter should be const #981

jduncanator opened this issue Jul 31, 2022 · 3 comments
Labels
Milestone

Comments

@jduncanator
Copy link

jduncanator commented Jul 31, 2022

Summary

The generated bindings for vkCmdUpdateBuffer have the pData parameter marked as ref (or Span<T> for the Span overloads), rather than in or ReadOnlySpan<T>. The official documentation for vkCmdUpdateBuffer has the pData parameter marked const.

These parameters should be treated like other const parameters, and take an in or ReadOnlySpan<T> argument.

Steps to reproduce

N/A

@jduncanator jduncanator added the bug Something isn't working label Jul 31, 2022
@jduncanator jduncanator changed the title CmdUpdateBuffer pData buffer should be const CmdUpdateBuffer pData parameter should be const Jul 31, 2022
@HurricanKai
Copy link
Member

While you're right from a C++ view, we currently generate C# bindings from the vk.xml file provided by khronos.
In addition using in has the drawback of allowing implicit usage, which has often shown to be a drawback, and we prefer ref wherever possible.
Using ROSpan would be preferable though, I doubt this will change in 2.x though, but 3.0 should be able to do this.

@HurricanKai HurricanKai added area-SilkTouch area-Vulkan enhancement New feature or request and removed bug Something isn't working labels Jul 31, 2022
@HurricanKai HurricanKai added this to the 3.0 milestone Jul 31, 2022
@Perksey
Copy link
Member

Perksey commented Dec 13, 2023

Just going through the 3.0 issue backlog. We have decided in the most recent working group meeting that following const correctness with the 3.0 style of overloading resulted in a significantly worse-off user experience and we are instead moving to an analyser-based const correctness approach.

So to address the actual issue here: SilkTouch 3.0 is already capable of recognising const correctness, and will annotate the function here. It will also likely be the case that you can pass a ReadOnlySpan into these functions, and the analyser warn if this is invalid to do.

To this end, I believe this issue is already as complete as it will be for 3.0, but I will leave it open until we've validated that we can indeed pass a ReadOnlySpan into these functions.

For more information, we encourage readers read the 3.0 proposals in #1727 (basically final, just need some minor edits) and/or watch the 3 hour livestream on the .NET Foundation YouTube channel in which we discussed our decisions in great detail.

Thoughts on the ReadOnlySpan castability @curin? I forget where we got on the SilkX effort here, but given the WG is already on board with the analyser based approach here, it wouldn't be an addition that goes against WG-approved precedents.

@curin
Copy link
Contributor

curin commented Dec 13, 2023

We didn't add the analysers yet and ReadOnlySpan cast wasn't added in the submitted version, only Span, but I think analysers will be important for cases like this, and adding a ReadOnlySpan override shouldn't be difficult for either Ref or Ptr types.

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

No branches or pull requests

4 participants