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

Fixed lerp() duplicate when compiling for C++20 #851

Merged
merged 3 commits into from
Mar 6, 2023

Conversation

niansa
Copy link
Contributor

@niansa niansa commented Mar 2, 2023

Hey!

I've noticed luau failing to compile on Clang:

luaaau/luau/VM/src/lmathlib.cpp:303: error: declaration conflicts with target of using declaration already in scope
luaaau/luau/VM/src/lmathlib.cpp:303:14: error: declaration conflicts with target of using declaration already in scope
static float lerp(float t, float a, float b)
             ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/cmath:1911:3: note: target of using declaration
  lerp(float __a, float __b, float __t) noexcept
  ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/math.h:183:12: note: using declaration
using std::lerp;
           ^

So I looked at the math.h file and found this:

#if __cplusplus > 201703L
using std::lerp;
#endif // C++20

So it seems like std::lerp() is put into the global namespace above C++17. That means that we already have a lerp() implementation in the global namespace! I decided to simply do this in luaus code:

#if __cplusplus <= 201703L
static float lerp(float t, float a, float b)
{
    return a + t * (b - a);
}
#endif

And voila, everything is compiling just fine now.

This probably isn't the cleanest solution, and I initially thought it simply was a Clang issue when I commited. Please comment your thoughts. 😄

Niansa

@niansa
Copy link
Contributor Author

niansa commented Mar 2, 2023

I wasn't able to test this on MSVC, just GCC and Clang. Does the CI run tests with C++20 selected?

@vegorov-rbx
Copy link
Collaborator

lerp and std::lerp actually have slightly different performance and precision 🙄
It would be better to rename lerp to math_lerp like we do to other functions in the file.

@niansa
Copy link
Contributor Author

niansa commented Mar 5, 2023

Here we go :-)

@vegorov-rbx
Copy link
Collaborator

Thank you!

@vegorov-rbx vegorov-rbx merged commit 78798d4 into luau-lang:master Mar 6, 2023
@niansa niansa deleted the patch-1 branch March 7, 2023 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants