Merge pull request #101 from acts-project/MSVCWarnings-main-20210913

Resolve MSVC Warnings, main branch (2021.09.13.)
acts-project · Sep 14, 2021 · 6291fcc · 6291fcc
2 parents d4b1b50 + ae7ce7f
commit 6291fcc
Show file tree

Hide file tree

Showing 15 changed files with 150 additions and 100 deletions.
diff --git a/cmake/vecmem-compiler-options-cpp.cmake b/cmake/vecmem-compiler-options-cpp.cmake
@@ -30,4 +30,15 @@ if( ( "${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU" ) OR
    # More rigorous tests for the Debug builds.
    vecmem_add_flag( CMAKE_CXX_FLAGS_DEBUG "-Werror" )
    vecmem_add_flag( CMAKE_CXX_FLAGS_DEBUG "-pedantic" )
+
+elseif( "${CMAKE_CXX_COMPILER_ID}" MATCHES "MSVC" )
+
+   # Basic flags for all build modes.
+   foreach( mode RELEASE RELWITHDEBINFO MINSIZEREL DEBUG )
+      vecmem_add_flag( CMAKE_CXX_FLAGS_${mode} "/W4" )
+   endforeach()
+
+   # More rigorous tests for the Debug builds.
+   vecmem_add_flag( CMAKE_CXX_FLAGS_DEBUG "/WX" )
+
 endif()
diff --git a/core/include/vecmem/containers/impl/jagged_vector.ipp b/core/include/vecmem/containers/impl/jagged_vector.ipp
@@ -24,12 +24,14 @@ data::jagged_vector_data<TYPE> get_data(jagged_vector<TYPE>& vec,
         size,
         (resource != nullptr ? *resource : *(vec.get_allocator().resource())));
 
-    // Helper local type definition.
+    // Helper local type definition(s).
     typedef typename data::jagged_vector_data<TYPE>::value_type value_type;
+    typedef typename value_type::size_type size_type;
 
     // Fill the result object with information.
     for (std::size_t i = 0; i < size; ++i) {
-        result.m_ptr[i] = value_type(vec[i].size(), vec[i].data());
+        result.m_ptr[i] =
+            value_type(static_cast<size_type>(vec[i].size()), vec[i].data());
     }
 
     // Return the created object.
@@ -50,12 +52,14 @@ data::jagged_vector_data<TYPE> get_data(
     // Construct the object to be returned.
     data::jagged_vector_data<TYPE> result(size, *resource);
 
-    // Helper local type definition.
+    // Helper local type definition(s).
     typedef typename data::jagged_vector_data<TYPE>::value_type value_type;
+    typedef typename value_type::size_type size_type;
 
     // Fill the result object with information.
     for (std::size_t i = 0; i < size; ++i) {
-        result.m_ptr[i] = value_type(vec[i].size(), vec[i].data());
+        result.m_ptr[i] =
+            value_type(static_cast<size_type>(vec[i].size()), vec[i].data());
     }
 
     // Return the created object.
@@ -74,13 +78,15 @@ data::jagged_vector_data<const TYPE> get_data(const jagged_vector<TYPE>& vec,
         size,
         (resource != nullptr ? *resource : *(vec.get_allocator().resource())));
 
-    // Helper local type definition.
+    // Helper local type definition(s).
     typedef
         typename data::jagged_vector_data<const TYPE>::value_type value_type;
+    typedef typename value_type::size_type size_type;
 
     // Fill the result object with information.
     for (std::size_t i = 0; i < size; ++i) {
-        result.m_ptr[i] = value_type(vec[i].size(), vec[i].data());
+        result.m_ptr[i] =
+            value_type(static_cast<size_type>(vec[i].size()), vec[i].data());
     }
 
     // Return the created object.
@@ -101,13 +107,15 @@ data::jagged_vector_data<const TYPE> get_data(
     // Construct the object to be returned.
     data::jagged_vector_data<const TYPE> result(size, *resource);
 
-    // Helper local type definition.
+    // Helper local type definition(s).
     typedef
         typename data::jagged_vector_data<const TYPE>::value_type value_type;
+    typedef typename value_type::size_type size_type;
 
     // Fill the result object with information.
     for (std::size_t i = 0; i < size; ++i) {
-        result.m_ptr[i] = value_type(vec[i].size(), vec[i].data());
+        result.m_ptr[i] =
+            value_type(static_cast<size_type>(vec[i].size()), vec[i].data());
     }
 
     // Return the created object.

diff --git a/core/include/vecmem/containers/impl/jagged_vector_buffer.ipp b/core/include/vecmem/containers/impl/jagged_vector_buffer.ipp
@@ -55,7 +55,9 @@ allocate_jagged_buffer_inner_memory(const std::vector<std::size_t>& sizes,
                                     bool isResizable) {
 
     typename vecmem::data::jagged_vector_buffer<TYPE>::size_type byteSize =
-        std::accumulate(sizes.begin(), sizes.end(), 0) * sizeof(TYPE);
+        std::accumulate(sizes.begin(), sizes.end(),
+                        static_cast<std::size_t>(0)) *
+        sizeof(TYPE);
     if (isResizable) {
         byteSize +=
             sizes.size() * sizeof(typename vecmem::data::jagged_vector_buffer<
@@ -103,8 +105,9 @@ jagged_vector_buffer<TYPE>::jagged_vector_buffer(
     // Set up the host accessible memory array.
     std::ptrdiff_t ptrdiff = 0;
     for (std::size_t i = 0; i < sizes.size(); ++i) {
-        new (host_ptr() + i) value_type(
-            sizes[i], reinterpret_cast<TYPE*>(m_inner_memory.get() + ptrdiff));
+        new (host_ptr() + i)
+            value_type(static_cast<typename value_type::size_type>(sizes[i]),
+                       reinterpret_cast<TYPE*>(m_inner_memory.get() + ptrdiff));
         ptrdiff += sizes[i] * sizeof(TYPE);
     }
 }
@@ -136,12 +139,12 @@ jagged_vector_buffer<TYPE>::jagged_vector_buffer(
     std::ptrdiff_t ptrdiff =
         (capacities.size() * sizeof(typename value_type::size_type));
     for (std::size_t i = 0; i < capacities.size(); ++i) {
-        new (host_ptr() + i)
-            value_type(capacities[i],
-                       reinterpret_cast<typename value_type::size_pointer>(
-                           m_inner_memory.get() +
-                           i * sizeof(typename value_type::size_type)),
-                       reinterpret_cast<TYPE*>(m_inner_memory.get() + ptrdiff));
+        new (host_ptr() + i) value_type(
+            static_cast<typename value_type::size_type>(capacities[i]),
+            reinterpret_cast<typename value_type::size_pointer>(
+                m_inner_memory.get() +
+                i * sizeof(typename value_type::size_type)),
+            reinterpret_cast<TYPE*>(m_inner_memory.get() + ptrdiff));
         ptrdiff += capacities[i] * sizeof(TYPE);
     }
 }

diff --git a/tests/core/test_core_array.cpp b/tests/core/test_core_array.cpp
@@ -47,32 +47,33 @@ class core_array_test
     void test_array(vecmem::array<T, N>& a) {
 
         // Fill the array with some simple values.
-        for (std::size_t i = 0; i < a.size(); ++i) {
+        for (int i = 0; i < static_cast<int>(a.size()); ++i) {
             a.at(i) = T(i);
         }
 
         // Check the contents using iterator based loops.
         {
             auto itr = a.begin();
-            for (std::size_t i = 0; itr != a.end(); ++itr, ++i) {
+            for (int i = 0; itr != a.end(); ++itr, ++i) {
                 EXPECT_EQ(*itr, T(i));
             }
             auto ritr = a.rbegin();
-            for (std::size_t i = a.size() - 1; ritr != a.rend(); ++ritr, --i) {
+            for (int i = static_cast<int>(a.size() - 1); ritr != a.rend();
+                 ++ritr, --i) {
                 EXPECT_EQ(*ritr, T(i));
             }
         }
 
         // Check its contents using a range based loop.
         {
-            std::size_t i = 0;
+            int i = 0;
             for (T value : a) {
                 EXPECT_EQ(value, T(i++));
             }
         }
 
         // Fill the array with a constant value.
-        static constexpr std::size_t VALUE = 123;
+        static constexpr int VALUE = 123;
         a.fill(T(VALUE));
         for (T value : a) {
             EXPECT_EQ(value, T(VALUE));

diff --git a/tests/core/test_core_containers.cpp b/tests/core/test_core_containers.cpp
@@ -40,15 +40,17 @@ TEST_F(core_container_test, const_device_vector) {
 
     const vecmem::const_device_vector<int> test_vector(
         vecmem::get_data(m_reference_vector));
-    EXPECT_TRUE(test_vector.size() == m_reference_vector.size());
-    EXPECT_TRUE(test_vector.empty() == m_reference_vector.empty());
+    EXPECT_EQ(test_vector.size(), m_reference_vector.size());
+    EXPECT_EQ(test_vector.empty(), m_reference_vector.empty());
     EXPECT_TRUE(std::equal(m_reference_vector.begin(), m_reference_vector.end(),
                            test_vector.begin()));
-    EXPECT_TRUE(std::accumulate(test_vector.begin(), test_vector.end(), 0) ==
-                std::accumulate(test_vector.rbegin(), test_vector.rend(), 0));
+    EXPECT_EQ(std::accumulate(test_vector.begin(), test_vector.end(), 0),
+              std::accumulate(test_vector.rbegin(), test_vector.rend(), 0));
     for (std::size_t i = 0; i < m_reference_vector.size(); ++i) {
-        EXPECT_TRUE(test_vector.at(i) == m_reference_vector.at(i));
-        EXPECT_TRUE(test_vector[i] == m_reference_vector[i]);
+        const vecmem::const_device_vector<int>::size_type ii =
+            static_cast<vecmem::const_device_vector<int>::size_type>(i);
+        EXPECT_EQ(test_vector.at(ii), m_reference_vector.at(i));
+        EXPECT_EQ(test_vector[ii], m_reference_vector[i]);
     }
 }
 
@@ -57,15 +59,17 @@ TEST_F(core_container_test, device_vector) {
 
     const vecmem::device_vector<int> test_vector(
         vecmem::get_data(m_reference_vector));
-    EXPECT_TRUE(test_vector.size() == m_reference_vector.size());
-    EXPECT_TRUE(test_vector.empty() == m_reference_vector.empty());
+    EXPECT_EQ(test_vector.size(), m_reference_vector.size());
+    EXPECT_EQ(test_vector.empty(), m_reference_vector.empty());
     EXPECT_TRUE(std::equal(m_reference_vector.begin(), m_reference_vector.end(),
                            test_vector.begin()));
-    EXPECT_TRUE(std::accumulate(test_vector.begin(), test_vector.end(), 0) ==
-                std::accumulate(test_vector.rbegin(), test_vector.rend(), 0));
+    EXPECT_EQ(std::accumulate(test_vector.begin(), test_vector.end(), 0),
+              std::accumulate(test_vector.rbegin(), test_vector.rend(), 0));
     for (std::size_t i = 0; i < m_reference_vector.size(); ++i) {
-        EXPECT_TRUE(test_vector.at(i) == m_reference_vector.at(i));
-        EXPECT_TRUE(test_vector[i] == m_reference_vector[i]);
+        const vecmem::const_device_vector<int>::size_type ii =
+            static_cast<vecmem::const_device_vector<int>::size_type>(i);
+        EXPECT_EQ(test_vector.at(ii), m_reference_vector.at(i));
+        EXPECT_EQ(test_vector[ii], m_reference_vector[i]);
     }
 }
 
@@ -95,15 +99,15 @@ TEST_F(core_container_test, const_device_array) {
 
     const vecmem::const_device_array<int, 9> test_array(
         vecmem::get_data(m_reference_vector));
-    EXPECT_TRUE(test_array.size() == m_reference_vector.size());
-    EXPECT_TRUE(test_array.empty() == m_reference_vector.empty());
+    EXPECT_EQ(test_array.size(), m_reference_vector.size());
+    EXPECT_EQ(test_array.empty(), m_reference_vector.empty());
     EXPECT_TRUE(std::equal(m_reference_vector.begin(), m_reference_vector.end(),
                            test_array.begin()));
-    EXPECT_TRUE(std::accumulate(test_array.begin(), test_array.end(), 0) ==
-                std::accumulate(test_array.rbegin(), test_array.rend(), 0));
+    EXPECT_EQ(std::accumulate(test_array.begin(), test_array.end(), 0),
+              std::accumulate(test_array.rbegin(), test_array.rend(), 0));
     for (std::size_t i = 0; i < m_reference_vector.size(); ++i) {
-        EXPECT_TRUE(test_array.at(i) == m_reference_vector.at(i));
-        EXPECT_TRUE(test_array[i] == m_reference_vector[i]);
+        EXPECT_EQ(test_array.at(i), m_reference_vector.at(i));
+        EXPECT_EQ(test_array[i], m_reference_vector[i]);
     }
 }
 
@@ -112,15 +116,15 @@ TEST_F(core_container_test, device_array) {
 
     const vecmem::device_array<int, 9> test_array(
         vecmem::get_data(m_reference_vector));
-    EXPECT_TRUE(test_array.size() == m_reference_vector.size());
-    EXPECT_TRUE(test_array.empty() == m_reference_vector.empty());
+    EXPECT_EQ(test_array.size(), m_reference_vector.size());
+    EXPECT_EQ(test_array.empty(), m_reference_vector.empty());
     EXPECT_TRUE(std::equal(m_reference_vector.begin(), m_reference_vector.end(),
                            test_array.begin()));
-    EXPECT_TRUE(std::accumulate(test_array.begin(), test_array.end(), 0) ==
-                std::accumulate(test_array.rbegin(), test_array.rend(), 0));
+    EXPECT_EQ(std::accumulate(test_array.begin(), test_array.end(), 0),
+              std::accumulate(test_array.rbegin(), test_array.rend(), 0));
     for (std::size_t i = 0; i < m_reference_vector.size(); ++i) {
-        EXPECT_TRUE(test_array.at(i) == m_reference_vector.at(i));
-        EXPECT_TRUE(test_array[i] == m_reference_vector[i]);
+        EXPECT_EQ(test_array.at(i), m_reference_vector.at(i));
+        EXPECT_EQ(test_array[i], m_reference_vector[i]);
     }
 }
 

diff --git a/tests/core/test_core_contiguous_memory_resource.cpp b/tests/core/test_core_contiguous_memory_resource.cpp
@@ -30,6 +30,12 @@ class core_contiguous_memory_resource_test : public testing::Test {
 /// Test the dumb allocation of a bunch of vectors
 TEST_F(core_contiguous_memory_resource_test, allocations) {
 
+// Skip this test with MSVC, in debug builds. As MSVC pads vectors in a way
+// that makes this test fail.
+#if defined(_MSC_VER) && defined(_DEBUG)
+    GTEST_SKIP();
+#else
+
     /// Fixed size for the test vectors
     static constexpr std::size_t VECTOR_SIZE = 100;
 
@@ -38,18 +44,20 @@ TEST_F(core_contiguous_memory_resource_test, allocations) {
     vecmem::vector<int> vec1(VECTOR_SIZE, &m_resource);
 
     vecmem::vector<char> vec2(VECTOR_SIZE, &m_resource);
-    EXPECT_TRUE(static_cast<void*>(&*(vec2.begin())) ==
-                static_cast<void*>(&*(vec1.end())));
+    EXPECT_EQ(static_cast<void*>(&*(vec2.begin())),
+              static_cast<void*>(&*(vec1.end())));
 
     vecmem::vector<double> vec3(VECTOR_SIZE, &m_resource);
-    EXPECT_TRUE(static_cast<void*>(&*(vec3.begin())) ==
-                static_cast<void*>(&*(vec2.end())));
+    EXPECT_EQ(static_cast<void*>(&*(vec3.begin())),
+              static_cast<void*>(&*(vec2.end())));
 
     vecmem::vector<float> vec4(VECTOR_SIZE, &m_resource);
-    EXPECT_TRUE(static_cast<void*>(&*(vec4.begin())) ==
-                static_cast<void*>(&*(vec3.end())));
+    EXPECT_EQ(static_cast<void*>(&*(vec4.begin())),
+              static_cast<void*>(&*(vec3.end())));
 
     vecmem::vector<int> vec5(VECTOR_SIZE, &m_resource);
-    EXPECT_TRUE(static_cast<void*>(&*(vec5.begin())) ==
-                static_cast<void*>(&*(vec4.end())));
+    EXPECT_EQ(static_cast<void*>(&*(vec5.begin())),
+              static_cast<void*>(&*(vec4.end())));
+
+#endif  // MSVC debug build...
 }
diff --git a/tests/core/test_core_device_containers.cpp b/tests/core/test_core_device_containers.cpp
@@ -128,10 +128,16 @@ TEST_F(core_device_container_test, resizable_vector_buffer) {
     // Create an input vector in regular host memory.
     std::vector<int> host_vector{1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
 
+    // Helper local type definitions.
+    typedef
+        typename vecmem::data::vector_buffer<int>::size_type buffer_size_type;
+    typedef typename vecmem::device_vector<int>::size_type vector_size_type;
+
     // Create a resizable buffer from that data.
-    static constexpr std::size_t BUFFER_SIZE = 100;
+    static constexpr buffer_size_type BUFFER_SIZE = 100;
     vecmem::data::vector_buffer<int> resizable_buffer(
-        BUFFER_SIZE, host_vector.size(), m_resource);
+        BUFFER_SIZE, static_cast<buffer_size_type>(host_vector.size()),
+        m_resource);
     m_copy.setup(resizable_buffer);
     EXPECT_EQ(resizable_buffer.capacity(), BUFFER_SIZE);
     m_copy(vecmem::get_data(host_vector), resizable_buffer);
@@ -151,45 +157,45 @@ TEST_F(core_device_container_test, resizable_vector_buffer) {
     // Modify the device vector in different ways, and check that it would work
     // as expected.
     device_vector.clear();
-    EXPECT_EQ(device_vector.size(), 0);
+    EXPECT_EQ(device_vector.size(), static_cast<vector_size_type>(0));
 
     device_vector.push_back(10);
-    EXPECT_EQ(device_vector.size(), 1);
+    EXPECT_EQ(device_vector.size(), static_cast<vector_size_type>(1));
     EXPECT_EQ(device_vector.at(0), 10);
 
     device_vector.emplace_back(15);
-    EXPECT_EQ(device_vector.size(), 2);
+    EXPECT_EQ(device_vector.size(), static_cast<vector_size_type>(2));
     EXPECT_EQ(device_vector.back(), 15);
 
     device_vector.assign(20, 123);
-    EXPECT_EQ(device_vector.size(), 20);
+    EXPECT_EQ(device_vector.size(), static_cast<vector_size_type>(20));
     for (int value : device_vector) {
         EXPECT_EQ(value, 123);
     }
 
     device_vector.resize(40, 234);
-    EXPECT_EQ(device_vector.size(), 40);
-    for (std::size_t i = 0; i < 20; ++i) {
+    EXPECT_EQ(device_vector.size(), static_cast<vector_size_type>(40));
+    for (vector_size_type i = 0; i < 20; ++i) {
         EXPECT_EQ(device_vector[i], 123);
     }
-    for (std::size_t i = 20; i < 40; ++i) {
+    for (vector_size_type i = 20; i < 40; ++i) {
         EXPECT_EQ(device_vector[i], 234);
     }
     device_vector.resize(25);
-    EXPECT_EQ(device_vector.size(), 25);
-    for (std::size_t i = 0; i < 20; ++i) {
+    EXPECT_EQ(device_vector.size(), static_cast<vector_size_type>(25));
+    for (vector_size_type i = 0; i < 20; ++i) {
         EXPECT_EQ(device_vector[i], 123);
     }
-    for (std::size_t i = 20; i < 25; ++i) {
+    for (vector_size_type i = 20; i < 25; ++i) {
         EXPECT_EQ(device_vector[i], 234);
     }
 
     device_vector.pop_back();
-    EXPECT_EQ(device_vector.size(), 24);
-    for (std::size_t i = 0; i < 20; ++i) {
+    EXPECT_EQ(device_vector.size(), static_cast<vector_size_type>(24));
+    for (vector_size_type i = 0; i < 20; ++i) {
         EXPECT_EQ(device_vector[i], 123);
     }
-    for (std::size_t i = 20; i < 24; ++i) {
+    for (vector_size_type i = 20; i < 24; ++i) {
         EXPECT_EQ(device_vector[i], 234);
     }