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

Inconsistent API and unnecessary mut qualifier. #1956

Closed
wdanilo opened this issue Jan 15, 2020 · 5 comments · Fixed by #1957
Closed

Inconsistent API and unnecessary mut qualifier. #1956

wdanilo opened this issue Jan 15, 2020 · 5 comments · Fixed by #1957
Labels

Comments

@wdanilo
Copy link
Contributor

wdanilo commented Jan 15, 2020

Hi! See the signatures of all uniform_matrixXXX_with_f32_array functions. Almost all of them consume data: &[f32]. However, the ones which are defined for non-square matrices, consume data: &mut [f32] instead. To keep the API consistent, all of them should consume data: &[f32].

@wdanilo wdanilo added the bug label Jan 15, 2020
@alexcrichton
Copy link
Contributor

Thanks for the report!

By default we assume mutable access is needed (since we don't know otherwise) and we have a whitelist of methods that don't mutate. If you'd like a PR to extend that list would be most welcome!

@wdanilo
Copy link
Contributor Author

wdanilo commented Jan 15, 2020

@alexcrichton sure thing, here it is: #1957 :)

As this is a very minor change, I'd be thankful if it could be merged fast :)

@wdanilo
Copy link
Contributor Author

wdanilo commented Jan 15, 2020

Oh, one more question!
There is another issue - the generated functions are not specialized for all possible variants. There is function tex_image_2d_with_i32_and_i32_and_i32_and_format_and_type_and_opt_u8_array but there are not functions for other array types (sure we can do it via the JsObject casting). According to MDN docs we can provide much more types here.

Would you be so nice and tell me where can I fix it in the code? :)

@alexcrichton
Copy link
Contributor

Ah unfortunately that's a lot more complicated since it's all based on the WebIDL that we're reading. Want to open a dedicated issue for that?

@wdanilo
Copy link
Contributor Author

wdanilo commented Jan 16, 2020

@alexandrestein of course!

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

Successfully merging a pull request may close this issue.

2 participants