- refactor public/implementation types into separate headers

- throw on overflow of distance integer type - use uint8 type for distance counts
ssciwr · Jun 22, 2021 · a86ec8d · a86ec8d
1 parent 717a398
commit a86ec8d
Show file tree

Hide file tree

Showing 12 changed files with 83 additions and 33 deletions.
diff --git a/include/hamming/hamming.hh b/include/hamming/hamming.hh
@@ -1,24 +1,12 @@
 #ifndef _HAMMING_HH
 #define _HAMMING_HH
 
-#include<array>
-#include<cstdint>
+#include"hamming/hamming_types.hh"
 #include<string>
 #include<vector>
 
 namespace hamming {
 
-struct DataSet
-{
-  DataSet(std::vector<std::string>&, bool clear_input_data = false);
-  DataSet(const std::string&);
-  void dump(const std::string&);
-  int operator[](const std::array<std::size_t, 2>&) const;
-
-  std::size_t nsamples;
-  std::vector<uint16_t> result;
-};
-
 DataSet from_stringlist(std::vector<std::string>&);
 DataSet from_csv(const std::string&);
 DataSet from_fasta(const std::string&, std::size_t n = 0);

diff --git a/include/hamming/hamming_types.hh b/include/hamming/hamming_types.hh
@@ -0,0 +1,26 @@
+#ifndef _HAMMING_TYPES_HH
+#define _HAMMING_TYPES_HH
+
+#include<array>
+#include<cstdint>
+#include<string>
+#include<vector>
+
+namespace hamming {
+
+using DistIntType = uint8_t;
+
+struct DataSet
+{
+  DataSet(std::vector<std::string>&, bool clear_input_data = false);
+  DataSet(const std::string&);
+  void dump(const std::string&);
+  int operator[](const std::array<std::size_t, 2>&) const;
+
+  std::size_t nsamples;
+  std::vector<DistIntType> result;
+};
+
+}
+
+#endif
diff --git a/src/distance_avx2.cc b/src/distance_avx2.cc
@@ -1,5 +1,5 @@
 #include "distance_avx2.hh"
-#include "hamming_impl.hh"
+#include "hamming_impl_types.hh"
 #include <immintrin.h>
 
 namespace hamming {

diff --git a/src/distance_avx2.hh b/src/distance_avx2.hh
@@ -4,9 +4,11 @@
 #include <cstdint>
 #include <vector>
 
+#include "hamming_impl_types.hh"
+
 namespace hamming {
 
-int distance_avx2(const std::vector<std::uint8_t>& a, const std::vector<std::uint8_t>& b);
+int distance_avx2(const std::vector<GeneBlock>& a, const std::vector<GeneBlock>& b);
 
 }
 

diff --git a/src/distance_avx512.cc b/src/distance_avx512.cc
@@ -1,5 +1,4 @@
 #include "distance_avx512.hh"
-#include "hamming_impl.hh"
 #include <immintrin.h>
 
 namespace hamming {

diff --git a/src/distance_avx512.hh b/src/distance_avx512.hh
@@ -4,9 +4,11 @@
 #include <cstdint>
 #include <vector>
 
+#include "hamming_impl_types.hh"
+
 namespace hamming {
 
-int distance_avx512(const std::vector<std::uint8_t>& a, const std::vector<std::uint8_t>& b);
+int distance_avx512(const std::vector<GeneBlock>& a, const std::vector<GeneBlock>& b);
 
 }
 

diff --git a/src/distance_sse2.cc b/src/distance_sse2.cc
@@ -1,5 +1,4 @@
 #include "distance_sse2.hh"
-#include "hamming_impl.hh"
 #include <immintrin.h>
 
 namespace hamming {

diff --git a/src/distance_sse2.hh b/src/distance_sse2.hh
@@ -4,9 +4,11 @@
 #include <cstdint>
 #include <vector>
 
+#include "hamming_impl_types.hh"
+
 namespace hamming {
 
-int distance_sse2(const std::vector<std::uint8_t>& a, const std::vector<std::uint8_t>& b);
+int distance_sse2(const std::vector<GeneBlock>& a, const std::vector<GeneBlock>& b);
 
 }
 

diff --git a/src/hamming_impl.cc b/src/hamming_impl.cc
@@ -37,8 +37,15 @@ std::array<GeneBlock, 256> lookupTable()
   return lookup;
 }
 
-std::vector<uint16_t> distances(std::vector<std::string>& data, bool clear_input_data){
-  std::vector<uint16_t> result((data.size() - 1) * data.size()/2, 0);
+static DistIntType safe_int_cast(int x){
+    if(x > std::numeric_limits<DistIntType>::max()){
+        throw std::runtime_error("Error: Distance is too large for chosen integer type");
+    }
+    return static_cast<DistIntType>(x);
+}
+
+std::vector<DistIntType> distances(std::vector<std::string>& data, bool clear_input_data){
+  std::vector<DistIntType> result((data.size() - 1) * data.size()/2, 0);
   auto sparse = to_sparse_data(data);
   std::size_t nsamples{data.size()};
   std::size_t sample_length{data[0].size()};
@@ -59,8 +66,9 @@ std::vector<uint16_t> distances(std::vector<std::string>& data, bool clear_input
     #endif
       for(std::size_t i=0; i<nsamples; ++i){
         std::size_t offset{i * (i - 1) / 2};
-        for(std::size_t j=0; j<i; ++j)
-          result[offset + j] = static_cast<uint16_t>(distance_sparse(sparse[i], sparse[j]));
+        for(std::size_t j=0; j<i; ++j){
+          result[offset + j] = safe_int_cast(distance_sparse(sparse[i], sparse[j]));
+        }
       }
       return result;
   }
@@ -93,7 +101,7 @@ std::vector<uint16_t> distances(std::vector<std::string>& data, bool clear_input
   for(std::size_t i=0; i<nsamples; ++i){
     std::size_t offset{i * (i - 1) / 2};
     for(std::size_t j=0; j<i; ++j)
-      result[offset + j] = static_cast<uint16_t>(distance_func(dense[i], dense[j]));
+      result[offset + j] = safe_int_cast(distance_func(dense[i], dense[j]));
   }
   return result;
 }

diff --git a/src/hamming_impl.hh b/src/hamming_impl.hh
@@ -6,18 +6,14 @@
 #include<string>
 #include<vector>
 
-namespace hamming {
+#include"hamming/hamming_types.hh"
+#include"hamming_impl_types.hh"
 
-// 4-bit representation of gene:
-using GeneBlock = std::uint_fast8_t;
-using SparseData = std::vector<std::size_t>;
-constexpr std::size_t n_bits_per_gene{4};
-constexpr GeneBlock mask_gene0{0x0f};
-constexpr GeneBlock mask_gene1{0xf0};
+namespace hamming {
 
 std::array<GeneBlock, 256> lookupTable();
 
-std::vector<uint16_t> distances(std::vector<std::string>& data, bool clear_input_data);
+std::vector<DistIntType> distances(std::vector<std::string>& data, bool clear_input_data);
 
 int distance_sparse(const SparseData& a, const SparseData& b);
 

diff --git a/src/hamming_impl_types.hh b/src/hamming_impl_types.hh
@@ -0,0 +1,18 @@
+#ifndef _HAMMING_IMPL_TYPES_HH
+#define _HAMMING_IMPL_TYPES_HH
+
+#include<array>
+#include<cstdint>
+
+namespace hamming {
+
+// 4-bit representation of gene:
+using GeneBlock = std::uint8_t;
+using SparseData = std::vector<std::size_t>;
+constexpr std::size_t n_bits_per_gene{4};
+constexpr GeneBlock mask_gene0{0x0f};
+constexpr GeneBlock mask_gene1{0xf0};
+
+}
+
+#endif
diff --git a/src/hamming_t.cc b/src/hamming_t.cc
@@ -167,7 +167,7 @@ TEST_CASE("from_csv reproduces correct data", "[hamming]") {
     std::mt19937 gen(12345);
     std::vector<std::string> data(10);
     for(auto& d : data)
-        d = make_test_string(1000, gen);
+        d = make_test_string(201, gen);
 
     DataSet ref(data);
     char tmp_file_name[L_tmpnam];
@@ -183,3 +183,13 @@ TEST_CASE("from_csv reproduces correct data", "[hamming]") {
     }
     std::remove(tmp_file_name);
 }
+
+TEST_CASE("throws on distance integer overflow", "[hamming]") {
+    auto n = std::numeric_limits<DistIntType>::max() + 1;
+    std::mt19937 gen(12345);
+    std::vector<std::string> data(2);
+    data[0] = std::string(n, 'A');
+    data[1] = std::string(n, 'T');
+    std::string msg{"Error: Distance is too large for chosen integer type"};
+    REQUIRE_THROWS_WITH(DataSet(data), msg);
+}