Fix and test bug in a FixedLenStringArray ctor. (#797)

BlueBrain · Jul 11, 2023 · 2c8b6b7 · 2c8b6b7
1 parent d2b1173
commit 2c8b6b7
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 16 deletions.
diff --git a/include/highfive/bits/H5DataType_misc.hpp b/include/highfive/bits/H5DataType_misc.hpp
@@ -330,18 +330,15 @@ inline FixedLenStringArray<N>::FixedLenStringArray(const char array[][N], std::s
 template <std::size_t N>
 inline FixedLenStringArray<N>::FixedLenStringArray(const std::string* iter_begin,
                                                    const std::string* iter_end) {
-    datavec.resize(static_cast<std::size_t>(iter_end - iter_begin));
-    for (auto& dst_array: datavec) {
-        const char* src = (iter_begin++)->c_str();
-        const size_t length = std::min(N - 1, std::strlen(src));
-        std::memcpy(dst_array.data(), src, length);
-        dst_array[length] = 0;
+    datavec.reserve(static_cast<std::size_t>(iter_end - iter_begin));
+    for (std::string const* it = iter_begin; it != iter_end; ++it) {
+        push_back(*it);
     }
 }
 
 template <std::size_t N>
 inline FixedLenStringArray<N>::FixedLenStringArray(const std::vector<std::string>& vec)
-    : FixedLenStringArray(&vec.front(), &vec.back()) {}
+    : FixedLenStringArray(vec.data(), vec.data() + vec.size()) {}
 
 template <std::size_t N>
 inline FixedLenStringArray<N>::FixedLenStringArray(

diff --git a/tests/unit/tests_high_five_base.cpp b/tests/unit/tests_high_five_base.cpp
@@ -2947,8 +2947,6 @@ TEST_CASE("HighFiveFixedString") {
 
     {  // Dedicated FixedLenStringArray
         FixedLenStringArray<10> arr{"0000000", "1111111"};
-        // For completeness, test also the other constructor
-        FixedLenStringArray<10> arrx(std::vector<std::string>{"0000", "1111"});
 
         // More API: test inserting something
         arr.push_back("2222");
@@ -3034,6 +3032,16 @@ TEST_CASE("HighFiveFixedString") {
     }
 }
 
+template <size_t N>
+static void check_fixed_len_string_array_contents(const FixedLenStringArray<N>& array,
+                                                  const std::vector<std::string>& expected) {
+    REQUIRE(array.size() == expected.size());
+
+    for (size_t i = 0; i < array.size(); ++i) {
+        CHECK(array[i] == expected[i]);
+    }
+}
+
 TEST_CASE("HighFiveFixedLenStringArrayStructure") {
     using fixed_array_t = FixedLenStringArray<10>;
     // increment the characters of a string written in a std::array
@@ -3048,16 +3056,52 @@ TEST_CASE("HighFiveFixedLenStringArrayStructure") {
         return output;
     };
 
+    SECTION("create from std::vector (onpoint)") {
+        auto expected = std::vector<std::string>{"000", "111"};
+        auto actual = FixedLenStringArray<4>(expected);
+        check_fixed_len_string_array_contents(actual, expected);
+    }
+
+    SECTION("create from std::vector (oversized)") {
+        auto expected = std::vector<std::string>{"000", "111"};
+        auto actual = FixedLenStringArray<8>(expected);
+        check_fixed_len_string_array_contents(actual, expected);
+    }
+
+    SECTION("create from pointers (onpoint)") {
+        auto expected = std::vector<std::string>{"000", "111"};
+        auto actual = FixedLenStringArray<4>(expected.data(), expected.data() + expected.size());
+        check_fixed_len_string_array_contents(actual, expected);
+    }
+
+    SECTION("create from pointers (oversized)") {
+        auto expected = std::vector<std::string>{"000", "111"};
+        auto actual = FixedLenStringArray<8>(expected.data(), expected.data() + expected.size());
+        check_fixed_len_string_array_contents(actual, expected);
+    }
+
+
+    SECTION("create from std::initializer_list (onpoint)") {
+        auto expected = std::vector<std::string>{"000", "111"};
+        auto actual = FixedLenStringArray<4>{"000", "111"};
+        check_fixed_len_string_array_contents(actual, expected);
+    }
+
+    SECTION("create from std::initializer_list (oversized)") {
+        auto expected = std::vector<std::string>{"000", "111"};
+        auto actual = FixedLenStringArray<8>{"000", "111"};
+        check_fixed_len_string_array_contents(actual, expected);
+    }
+
     // manipulate FixedLenStringArray with std::copy
-    {
+    SECTION("compatible with std::copy") {
         const fixed_array_t arr1{"0000000", "1111111"};
         fixed_array_t arr2{"0000000", "1111111"};
         std::copy(arr1.begin(), arr1.end(), std::back_inserter(arr2));
         CHECK(arr2.size() == 4);
     }
 
-    // manipulate FixedLenStringArray with std::transform
-    {
+    SECTION("compatible with std::transform") {
         fixed_array_t arr;
         {
             const fixed_array_t arr1{"0000000", "1111111"};
@@ -3068,8 +3112,7 @@ TEST_CASE("HighFiveFixedLenStringArrayStructure") {
         CHECK(arr[1] == std::string("2222222"));
     }
 
-    // manipulate FixedLenStringArray with std::transform and reverse iterator
-    {
+    SECTION("compatible with std::transform (reverse iterator)") {
         fixed_array_t arr;
         {
             const fixed_array_t arr1{"0000000", "1111111"};
@@ -3080,8 +3123,7 @@ TEST_CASE("HighFiveFixedLenStringArrayStructure") {
         CHECK(arr[1] == std::string("0000000"));
     }
 
-    // manipulate FixedLenStringArray with std::remove_copy_if
-    {
+    SECTION("compatible with std::remove_copy_if") {
         fixed_array_t arr2;
         {
             const fixed_array_t arr1{"0000000", "1111111"};