Deprecate FixedLenStringArray. (BlueBrain#932)

doc/Doxyfile

@@ -866,6 +866,7 @@ WARN_LOGFILE           =
 
 INPUT                  = @CMAKE_CURRENT_SOURCE_DIR@/../include \
                          @CMAKE_CURRENT_SOURCE_DIR@/installation.md \
+                         @CMAKE_CURRENT_SOURCE_DIR@/migration_guide.md \
                          @CMAKE_CURRENT_SOURCE_DIR@/developer_guide.md \
                          @CMAKE_CURRENT_SOURCE_DIR@/../CHANGELOG.md \
                          @CMAKE_CURRENT_SOURCE_DIR@/../README.md

doc/migration_guide.md

@@ -0,0 +1,16 @@
+# Migration Guide
+A collection of tips for migrating away from deprecated features.
+
+## Deprecation of `FixedLenStringArray`.
+The issue with `FixedLenStringArray` is that it is unable to avoid copies.
+Essentially, this class acts as a means to create a copy of the data in a
+format suitable for writing fixed-length strings. Additionally, the class acts
+as a tag for HighFive to overload on. The support of `std::string` in HighFive
+has improved considerable. Since 2.8.0 we can write/read `std::string` to fixed
+or variable length HDF5 strings.
+
+Therefore, this class serves no purpose anymore. Any occurrence of it can be
+replaced with an `std::vector<std::string>` (for example).
+
+If desired one can silence warnings by replacing `FixedLenStringArray` with
+`deprecated::FixedLenStringArray`.

include/highfive/H5DataType.hpp

@@ -342,6 +342,7 @@ template <typename T>
 DataType create_and_check_datatype();
 
 
+namespace deprecated {
 ///
 /// \brief A structure representing a set of fixed-length strings
 ///
@@ -460,6 +461,11 @@ class FixedLenStringArray {
   private:
     vector_t datavec;
 };
+}  // namespace deprecated
+
+template <size_t N>
+using FixedLenStringArray H5_DEPRECATED_USING("Use 'std::vector<std::string>'.") =
+    deprecated::FixedLenStringArray<N>;
 
 }  // namespace HighFive
 

include/highfive/bits/H5DataType_misc.hpp

@@ -207,7 +207,7 @@ class AtomicType<char[StrLen]>: public DataType {
 };
 
 template <size_t StrLen>
-class AtomicType<FixedLenStringArray<StrLen>>: public DataType {
+class AtomicType<deprecated::FixedLenStringArray<StrLen>>: public DataType {
   public:
     inline AtomicType()
         : DataType(create_string(StrLen)) {}
@@ -239,8 +239,7 @@ AtomicType<T>::AtomicType() {
 }
 
 
-// class FixedLenStringArray<N>
-
+namespace deprecated {
 template <std::size_t N>
 inline FixedLenStringArray<N>::FixedLenStringArray(const char array[][N], std::size_t length) {
     datavec.resize(length);
@@ -283,6 +282,7 @@ template <std::size_t N>
 inline std::string FixedLenStringArray<N>::getString(std::size_t i) const {
     return std::string(datavec[i].data());
 }
+}  // namespace deprecated
 
 // Internal
 // Reference mapping

include/highfive/bits/H5Inspector_misc.hpp

@@ -289,10 +289,10 @@ struct inspector<Reference>: type_helper<Reference> {
 };
 
 template <size_t N>
-struct inspector<FixedLenStringArray<N>> {
-    using type = FixedLenStringArray<N>;
+struct inspector<deprecated::FixedLenStringArray<N>> {
+    using type = deprecated::FixedLenStringArray<N>;
     using value_type = char*;
-    using base_type = FixedLenStringArray<N>;
+    using base_type = deprecated::FixedLenStringArray<N>;
     using hdf5_type = char;
 
     static constexpr size_t ndim = 1;

include/highfive/bits/H5Node_traits.hpp

@@ -79,8 +79,9 @@ class NodeTraits {
 
 
     template <std::size_t N>
+    H5_DEPRECATED("Use 'std::vector<std::string>'.")
     DataSet createDataSet(const std::string& dataset_name,
-                          const FixedLenStringArray<N>& data,
+                          const deprecated::FixedLenStringArray<N>& data,
                           const DataSetCreateProps& createProps = DataSetCreateProps::Default(),
                           const DataSetAccessProps& accessProps = DataSetAccessProps::Default(),
                           bool parents = true);

include/highfive/bits/H5Node_traits_misc.hpp

@@ -83,7 +83,7 @@ inline DataSet NodeTraits<Derivate>::createDataSet(const std::string& dataset_na
 template <typename Derivate>
 template <std::size_t N>
 inline DataSet NodeTraits<Derivate>::createDataSet(const std::string& dataset_name,
-                                                   const FixedLenStringArray<N>& data,
+                                                   const deprecated::FixedLenStringArray<N>& data,
                                                    const DataSetCreateProps& createProps,
                                                    const DataSetAccessProps& accessProps,
                                                    bool parents) {

include/highfive/bits/H5Utils.hpp

@@ -25,9 +25,11 @@
 
 namespace HighFive {
 
+namespace deprecated {
 // If ever used, recognize dimensions of FixedLenStringArray
 template <std::size_t N>
 class FixedLenStringArray;
+}  // namespace deprecated
 
 namespace details {
 // converter function for hsize_t -> size_t when hsize_t != size_t

include/highfive/bits/H5_definitions.hpp

@@ -5,10 +5,17 @@
 #elif defined(_MSC_VER)
 #define H5_DEPRECATED(msg) __declspec(deprecated(#msg))
 #else
-#pragma message("WARNING: Compiler doesnt support deprecation")
+#pragma message("WARNING: Compiler doesn't support deprecation")
 #define H5_DEPRECATED(msg)
 #endif
 
+#if defined(__GNUC__) || defined(__clang__)
+#define H5_DEPRECATED_USING(msg) H5_DEPRECATED((msg))
+#else
+#pragma message("WARNING: Compiler doesn't support deprecating using statements.")
+#define H5_DEPRECATED_USING(msg)
+#endif
+
 
 // Forward declarations
 
@@ -38,8 +45,10 @@ class AtomicType;
 template <typename Derivate>
 class AnnotateTraits;
 
+namespace deprecated {
 template <std::size_t N>
 class FixedLenStringArray;
+}
 
 template <typename Derivate>
 class NodeTraits;

tests/unit/CMakeLists.txt

@@ -52,3 +52,5 @@ if(HIGHFIVE_TEST_SINGLE_INCLUDES)
         target_link_libraries("tests_include_${CLASS_NAME}" HighFive HighFiveWarnings)
     endforeach()
 endif()
+
+add_subdirectory(deprecated)

tests/unit/deprecated/CMakeLists.txt

@@ -0,0 +1,10 @@
+foreach(test_name test_fixed_len_string_array)
+  add_executable(${test_name} "${test_name}.cpp")
+
+  target_link_libraries(${test_name} HighFive HighFiveWarnings Catch2::Catch2WithMain)
+  catch_discover_tests(${test_name})
+
+  if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID MATCHES "GNU")
+    target_compile_options(${test_name} PRIVATE -Wno-deprecated-declarations)
+  endif()
+endforeach()

tests/unit/deprecated/test_fixed_len_string_array.cpp

@@ -0,0 +1,172 @@
+#include <catch2/catch_test_macros.hpp>
+
+#include <highfive/highfive.hpp>
+#include "../tests_high_five.hpp"
+
+namespace HighFive {
+
+TEST_CASE("HighFiveFixedLenStringArray") {
+    const std::string file_name("fixed_len_string_array.h5");
+
+    // Create a new file using the default property lists.
+    File file(file_name, File::ReadWrite | File::Create | File::Truncate);
+
+    {  // Dedicated FixedLenStringArray (now deprecated).
+        FixedLenStringArray<10> arr{"0000000", "1111111"};
+
+        // More API: test inserting something
+        arr.push_back("2222");
+        auto ds = file.createDataSet("ds7", arr);  // Short syntax ok
+
+        // Recover truncating
+        FixedLenStringArray<4> array_back;
+        ds.read(array_back);
+        CHECK(array_back.size() == 3);
+        CHECK(array_back[0] == std::string("000"));
+        CHECK(array_back[1] == std::string("111"));
+        CHECK(array_back[2] == std::string("222"));
+        CHECK(array_back.getString(1) == "111");
+        CHECK(array_back.front() == std::string("000"));
+        CHECK(array_back.back() == std::string("222"));
+        CHECK(array_back.data() == std::string("000"));
+        array_back.data()[0] = 'x';
+        CHECK(array_back.data() == std::string("x00"));
+
+        for (auto& raw_elem: array_back) {
+            raw_elem[1] = 'y';
+        }
+        CHECK(array_back.getString(1) == "1y1");
+        for (auto iter = array_back.cbegin(); iter != array_back.cend(); ++iter) {
+            CHECK((*iter)[1] == 'y');
+        }
+    }
+}
+
+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
+    auto increment_string = [](const fixed_array_t::value_type arr) {
+        fixed_array_t::value_type output(arr);
+        for (auto& c: output) {
+            if (c == 0) {
+                break;
+            }
+            ++c;
+        }
+        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);
+    }
+
+    SECTION("compatible with std::transform") {
+        fixed_array_t arr;
+        {
+            const fixed_array_t arr1{"0000000", "1111111"};
+            std::transform(arr1.begin(), arr1.end(), std::back_inserter(arr), increment_string);
+        }
+        CHECK(arr.size() == 2);
+        CHECK(arr[0] == std::string("1111111"));
+        CHECK(arr[1] == std::string("2222222"));
+    }
+
+    SECTION("compatible with std::transform (reverse iterator)") {
+        fixed_array_t arr;
+        {
+            const fixed_array_t arr1{"0000000", "1111111"};
+            std::copy(arr1.rbegin(), arr1.rend(), std::back_inserter(arr));
+        }
+        CHECK(arr.size() == 2);
+        CHECK(arr[0] == std::string("1111111"));
+        CHECK(arr[1] == std::string("0000000"));
+    }
+
+    SECTION("compatible with std::remove_copy_if") {
+        fixed_array_t arr2;
+        {
+            const fixed_array_t arr1{"0000000", "1111111"};
+            std::remove_copy_if(arr1.begin(),
+                                arr1.end(),
+                                std::back_inserter(arr2),
+                                [](const fixed_array_t::value_type& s) {
+                                    return std::strncmp(s.data(), "1111111", 7) == 0;
+                                });
+        }
+        CHECK(arr2.size() == 1);
+        CHECK(arr2[0] == std::string("0000000"));
+    }
+}
+
+TEST_CASE("HighFiveFixedLenStringArrayAttribute") {
+    const std::string file_name("fixed_array_attr.h5");
+    // Create a new file using the default property lists.
+    {
+        File file(file_name, File::ReadWrite | File::Create | File::Truncate);
+        FixedLenStringArray<10> arr{"Hello", "world"};
+        file.createAttribute("str", arr);
+    }
+    // Re-read it
+    {
+        File file(file_name);
+        FixedLenStringArray<8> arr;  // notice the output strings can be smaller
+        file.getAttribute("str").read(arr);
+        CHECK(arr.size() == 2);
+        CHECK(arr[0] == std::string("Hello"));
+        CHECK(arr[1] == std::string("world"));
+    }
+}
+
+}  // namespace HighFive