23: Breaks up nearest tests to not be singular test

[khurd21]

Tests modified:

• floor_test.cpp
• trunc_test.cpp

Both adopts parameterized tests. These help reduce the expect calls and code duplication, also making it easier to add additional test cases in the future. NaN tests are still inside of the original TEST function.

Since the tests seemed to be directly comparing the function under test with the standard library equivalent, adjusted the std::signbit and std::isnan comparisons to directly compare equality in the NaN tests.

I also made sure to run the clang-format on the files that were modified.

[khurd21]

I saw this project and thought it was very well documented and heavily tested! I wanted to use this PR as an introduction and to see if changes like this for the tests are okay! I saw your card #23 . If these changes look okay, I would love to look into the other ones and format them similarly.

[Rinzii]

Hello! Firstly, thank you for taking the time to create a PR for ccmath!

Overall, these changes are perfectly acceptable the only request I would have is that somewhere in the test we perform a static_assert to ensure the functions are being evaluated at compile time, but overall these changes seem pretty good!

[Rinzii]

All that is required at the moment would be to add a static_assert to both of these test cases to ensure our functions work at compile time. Besides lgtm!

[khurd21]

I tried giving this a good tackle and think I stumbled upon an issue. I noticed when I added the tests for compile time assertions, theccm::truncl yieled an error for the ninja-clang build only. The error is here:

https://github.com/Rinzii/ccmath/actions/runs/9848897543/job/27191697093#step:5:82

It seems like there is an improper type being passed into __builtin_bit_cast based on the message. I was unable to replicate this locally. I attempted to use the ninja-clang configuration preset but I was unable to get a successful build yet.

In the most recent push, I commented out the internals of the test with the constexpr ccm::truncl and it seems there is no issue with any of the other functions in the nearest folder.

What are you thoughts on this?

[Rinzii]

I tried giving this a good tackle and think I stumbled upon an issue. I noticed when I added the tests for compile time assertions, theccm::truncl yieled an error for the ninja-clang build only. The error is here:

https://github.com/Rinzii/ccmath/actions/runs/9848897543/job/27191697093#step:5:82

It seems like there is an improper type being passed into __builtin_bit_cast based on the message. I was unable to replicate this locally. I attempted to use the ninja-clang configuration preset but I was unable to get a successful build yet.

In the most recent push, I commented out the internals of the test with the constexpr ccm::truncl and it seems there is no issue with any of the other functions in the nearest folder.

What are you thoughts on this?

Sorry I‘ve been having a lot of IRL stuff going on recently and have not had much time to look at my GitHub. Is this issue still persisting?

[khurd21]

I tried giving this a good tackle and think I stumbled upon an issue. I noticed when I added the tests for compile time assertions, theccm::truncl yieled an error for the ninja-clang build only. The error is here:
https://github.com/Rinzii/ccmath/actions/runs/9848897543/job/27191697093#step:5:82
It seems like there is an improper type being passed into __builtin_bit_cast based on the message. I was unable to replicate this locally. I attempted to use the ninja-clang configuration preset but I was unable to get a successful build yet.
In the most recent push, I commented out the internals of the test with the constexpr ccm::truncl and it seems there is no issue with any of the other functions in the nearest folder.
What are you thoughts on this?

Sorry I‘ve been having a lot of IRL stuff going on recently and have not had much time to look at my GitHub. Is this issue still persisting?

No problem. Yes, the issue still is persisting. I have been trying to build using clang on Ubuntu locally, however I was getting other build errors before this one. In the current commit, I have commented out the compile time test I added for ccm::truncl. It seems that truncl is the only function out of the nearest tests that does not work at compile time for Linux Ninja Clang build.

If this sounds like an okay plan, I leave commented out compile time test here and have a card to address this properly in the future?

[Rinzii]

I tried giving this a good tackle and think I stumbled upon an issue. I noticed when I added the tests for compile time assertions, theccm::truncl yieled an error for the ninja-clang build only. The error is here:
https://github.com/Rinzii/ccmath/actions/runs/9848897543/job/27191697093#step:5:82
It seems like there is an improper type being passed into __builtin_bit_cast based on the message. I was unable to replicate this locally. I attempted to use the ninja-clang configuration preset but I was unable to get a successful build yet.
In the most recent push, I commented out the internals of the test with the constexpr ccm::truncl and it seems there is no issue with any of the other functions in the nearest folder.
What are you thoughts on this?

Sorry I‘ve been having a lot of IRL stuff going on recently and have not had much time to look at my GitHub. Is this issue still persisting?

No problem. Yes, the issue still is persisting. I have been trying to build using clang on Ubuntu locally, however I was getting other build errors before this one. In the current commit, I have commented out the compile time test I added for ccm::truncl. It seems that truncl is the only function out of the nearest tests that does not work at compile time for Linux Ninja Clang build.

If this sounds like an okay plan, I leave commented out compile time test here and have a card to address this properly in the future?

Hmmm I’ll investigate this issue myself locally in a few days. I’ll see if I can identify what the problem may be. :)

[Rinzii]

Ok, after looking into this it appears the issue is because of specific implementation details of how __builtin_bit_cast works on clang vs GCC. This issue is appearing due to __builtin_bit_cast inability to determine the value of long double since it will always be 16 bytes, but can be represented as either 80 or 128 bits. GCC appears to just let it fly but clang does not and errors out. There is no real simple way to solve this issue, so for simplicity I'd just get rid of this test with a macro if we are on clang and testing truncl specifically.

[khurd21]

Ok, after looking into this it appears the issue is because of specific implementation details of how __builtin_bit_cast works on clang vs GCC. This issue is appearing due to __builtin_bit_cast inability to determine the value of long double since it will always be 16 bytes, but can be represented as either 80 or 128 bits. GCC appears to just let it fly but clang does not and errors out. There is no real simple way to solve this issue, so for simplicity I'd just get rid of this test with a macro if we are on clang and testing truncl specifically.

Sounds good! I updated the code. I added a macro to the cpp file here: https://github.com/Rinzii/ccmath/pull/43/files#diff-0ad15176bfaf60d1dd519b442c7412c099c1d2425808bd30e4823cf9ddd4722bR17

Since clang linux was the only one failing, I made sure to check the compiler was clang and the os was linux. I then undefined it at the end of the source file to try and reduce the scope of the macro to be just that test file.

[Rinzii]

lgtm!