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.

Sign up for GitHub

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

Jump to bottom

overall_dimensions_t should be constructible from a dim3 struct #404

Closed
eyalroz opened this issue Sep 9, 2022 · 4 comments
Closed

overall_dimensions_t should be constructible from a dim3 struct #404

eyalroz opened this issue Sep 9, 2022 · 4 comments
Assignees
@eyalroz
Labels
resolved-on-development task

Comments

@eyalroz
Copy link
Owner

eyalroz commented Sep 9, 2022

While overall_dimensions_t is a 3-dimension variable, with 3 size_t's, dim3 is not - it uses unsigned ints.

We should support construction using dim3's (but not the conversion back).
@eyalroz eyalroz added the task label Sep 9, 2022
@eyalroz eyalroz self-assigned this Sep 9, 2022
eyalroz added a commit that referenced this issue Sep 9, 2022
@eyalroz
Fixes #404: overall_dimensions_t implicit conversions/constructors …
7c2e9d0 
…added

* Replaced rvalue `dimensions_t` with plain value `dimensions_t` ctor (to be able to take lvalue references too)
* Added a `dim3` constructor (by-value)
@hofstee
Copy link

hofstee commented Sep 20, 2022

Shouldn't it be a dim3& or a const dim3& like the other places using a dim3 with constexpr? Otherwise with MSVC I get the following error: a constexpr function cannot have a parameter of nonliteral type "dim3".

@eyalroz
Copy link
Owner Author

eyalroz commented Sep 20, 2022 •
edited

@hofstee : I believe MSVC is wrong; constexpr function can definitely have parameters of more complex types, as long as their constructors are constexpr. But let's do it like you suggest to also caster to MSVC's "sensibilities".

@eyalroz
Copy link
Owner Author

eyalroz commented Sep 22, 2022

Done... @hofstee : Please have a look / give this a try.

@hofstee
Copy link

hofstee commented Sep 23, 2022

Seems to work on my end! Thanks!

@eyalroz eyalroz closed this as completed in 31e8682 Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eyalroz eyalroz

Labels
resolved-on-development task
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@hofstee @eyalroz