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

[WIP] Bugfix: quantity_cast fails to compile when provided with a lvalue reference #569

Merged

Conversation

burnpanck
Copy link
Contributor

No description provided.

@burnpanck burnpanck changed the title Bugfix: quantity_cast fails to compile when provided with a lvalue reference [WIP] Bugfix: quantity_cast fails to compile when provided with a lvalue reference May 10, 2024
@@ -937,7 +937,11 @@ static_assert(is_of_type<quantity_cast<isq::distance>(1 * m), quantity<isq::dist
static_assert(is_of_type<quantity_cast<isq::distance>(isq::length(1 * m)), quantity<isq::distance[m], int>>);
static_assert(is_of_type<quantity_cast<kind_of<isq::length>>(isq::length(1 * m)), quantity<si::metre, int>>);
static_assert(is_of_type<quantity_cast<kind_of<isq::length>>(isq::distance(1 * m)), quantity<si::metre, int>>);

// lvalue references in quantity_cast
inline constexpr quantity<isq::distance[m], int> to_distance(quantity<m, int> arg)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline is not needed here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I am not sure if we need a function here as it is not a test (although it has to be compiled correctly).
Maybe it would be better to define a quantity variable and pass it to a static_assert(). With this, you could check it the output type is exactly what you require as well.

test/static/quantity_test.cpp Outdated Show resolved Hide resolved
Co-authored-by: Mateusz Pusz <mateusz.pusz@gmail.com>
@mpusz
Copy link
Owner

mpusz commented May 10, 2024

This bug is really embarrassing. Thanks for finding and fixing it!

@mpusz mpusz merged commit 4c7f58e into mpusz:master May 10, 2024
41 of 77 checks passed
@mpusz
Copy link
Owner

mpusz commented May 10, 2024

BTW, CMake CI is broken now because the new Conan version is not compatible with the previous one. I have to fix it when I get some time. So do not be surprised by one failing GitHub action.

@burnpanck
Copy link
Contributor Author

Uh oh, I believe value_cast has the same issue; I'll try to fix that one too

@burnpanck burnpanck deleted the bugfix/allow-lvalue-references-in-quantity-cast branch May 10, 2024 21:04
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

Successfully merging this pull request may close these issues.

2 participants