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

#1236 - MSVC compile unit.api.exe #1467

Closed
wants to merge 8 commits into from
Closed

#1236 - MSVC compile unit.api.exe #1467

wants to merge 8 commits into from

Conversation

jfalcou
Copy link
Owner

@jfalcou jfalcou commented Nov 28, 2022

No description provided.

@jfalcou jfalcou force-pushed the msvc/api-fix branch 15 times, most recently from b5e5e14 to b3eed8b Compare November 29, 2022 16:58
Copy link
Collaborator

@DenisYaroshevskiy DenisYaroshevskiy left a comment

Choose a reason for hiding this comment

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

So far more or less OK. Please lemme know when you are done

)
, tts::generate ( tts::randoms(0, 100)
, tts::randoms(0, 100)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What - msvc cannot deal with negative numbers?

Copy link

Choose a reason for hiding this comment

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

The problem is that a uniform_int_distribution<> of unsigned integral type should not be constructed from negative integers, as they will implicitly be converted to unsigned and become very large. Therefore, leading to a>b which is undefined behavior: https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution/uniform_int_distribution

Copy link
Owner Author

Choose a reason for hiding this comment

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

The proper fix is maybe to split the test between signed and unsigned.

@jfalcou jfalcou force-pushed the msvc/api-fix branch 10 times, most recently from 858b0e9 to 53f5c2f Compare December 4, 2022 11:09
@Tradias
Copy link

Tradias commented Dec 5, 2022

Here is the patch for the ICE in deinterleave_groups.cpp. Some of the inability to capture non-type template parameter in lambdas is certainly a common MSVC issue, but this piece of code is not valid C++:

[](auto a)
{
   some_func<a()>();
}
(std::integral_constant<int, 42>{});

and cannot be blamed on MSVC^^.

diff --git a/test/unit/api/regular/deinterleave_groups.cpp b/test/unit/api/regular/deinterleave_groups.cpp
index 09bd6393f..57378c37e 100644
--- a/test/unit/api/regular/deinterleave_groups.cpp
+++ b/test/unit/api/regular/deinterleave_groups.cpp
@@ -114,19 +114,20 @@ using less_test_types =  eve::test::wides < tts::types< std::int8_t, std::uint16
 TTS_CASE_TPL( "Check behavior of deinterleave on arithmetic data", less_test_types)
 <typename T>(tts::type<T>)
 {
-  constexpr std::ptrdiff_t max_fields_count = 5;
+  static constexpr std::ptrdiff_t max_fields_count = 5;
 
-  eve::detail::for_<1, 1, max_fields_count + 1>([](auto fields)
+  eve::detail::for_<1, 1, max_fields_count + 1>([&]<int Fields>(std::integral_constant<int, Fields>)
   {
     // maybe unsused for gcc bug
     static constexpr std::size_t max_group_size = (T::size() >= 64) ? 4 : T::size();
-    constexpr auto nn = std::countr_zero(max_group_size) + 1;
+    static constexpr auto nn = std::countr_zero(max_group_size) + 1;
+    static constexpr std::ptrdiff_t fields = Fields;
     eve::detail::for_<0, 1, nn>
     (
-      [&](auto group_size_log)
+      [&]<int Shift>(std::integral_constant<int, Shift>)
       {
-        constexpr auto gs = 1 << group_size_log();
-        deinterleave_groups_test<gs, fields(), T>();
+        static constexpr auto gs = 1 << Shift;
+        deinterleave_groups_test<gs, fields, T>();
       }
     );
   });

@jfalcou
Copy link
Owner Author

jfalcou commented Dec 5, 2022

Great! Will add it to the current PR

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.

None yet

3 participants