From e5173a9490a6397dee78e7b87dcdeac8de7f4bbc Mon Sep 17 00:00:00 2001
From: Stephane Del Pino <stephane.delpino44@gmail.com>
Date: Thu, 30 Jul 2020 16:01:13 +0200
Subject: [PATCH] Improve elements access checking

Now one checks that A(i,j) is accessed with i being the correct
ItemId and j being an integral type. This avoids access such as
- A(i,j) with i being an integral type
- A(i,j) with j being an ItemId

Also add bounds checking for the second index `j` (previously the only
bound checking was if (i,j) was in the scope of the data set, checked
by Kokkos). One also check that `i` is a correct ItemId value, it was
not checked in all cases.
---
 src/mesh/SubItemValuePerItem.hpp | 86 +++++++++++++++-----------------
 1 file changed, 41 insertions(+), 45 deletions(-)

diff --git a/src/mesh/SubItemValuePerItem.hpp b/src/mesh/SubItemValuePerItem.hpp
index 9ce6369e0..8189fc13a 100644
--- a/src/mesh/SubItemValuePerItem.hpp
+++ b/src/mesh/SubItemValuePerItem.hpp
@@ -54,16 +54,20 @@ class SubItemValuePerItem
     const size_t m_size;
 
    public:
-    PUGS_INLINE
-    const DataType& operator[](size_t i) const noexcept(NO_ASSERT)
+    template <typename IndexType>
+    PUGS_INLINE const DataType&
+    operator[](IndexType i) const noexcept(NO_ASSERT)
     {
+      static_assert(std::is_integral_v<IndexType>, "SubView is indexed by integral values");
       Assert(i < m_size);
       return m_sub_values[i];
     }
 
-    PUGS_FORCEINLINE
-    DataType& operator[](size_t i) noexcept(NO_ASSERT)
+    template <typename IndexType>
+    PUGS_FORCEINLINE DataType&
+    operator[](IndexType i) noexcept(NO_ASSERT)
     {
+      static_assert(std::is_integral_v<IndexType>, "SubView is indexed by integral values");
       Assert(i < m_size);
       return m_sub_values[i];
     }
@@ -96,23 +100,15 @@ class SubItemValuePerItem
     return m_connectivity_ptr.use_count() != 0;
   }
 
-  // Following Kokkos logic, these classes are view and const view does allow
-  // changes in data
-  PUGS_FORCEINLINE
-  DataType&
-  operator()(ItemId j, size_t r) const noexcept(NO_ASSERT)
-  {
-    Assert(this->isBuilt());
-    return m_values[m_host_row_map(size_t{j}) + r];
-  }
-
-  template <typename IndexType>
+  template <typename IndexType, typename SubIndexType>
   PUGS_FORCEINLINE DataType&
-  operator()(IndexType j, size_t r) const noexcept(NO_ASSERT)
+  operator()(IndexType item_id, SubIndexType i) const noexcept(NO_ASSERT)
   {
-    static_assert(std::is_same_v<IndexType, ItemId>, "SubItemValuePerItem indexed by ItemId");
+    static_assert(std::is_same_v<IndexType, ItemId>, "first index must be an ItemId");
+    static_assert(std::is_integral_v<SubIndexType>, "second index must be an integral type");
     Assert(this->isBuilt());
-    return m_values[m_host_row_map(size_t{j}) + r];
+    Assert(i < m_host_row_map(size_t{item_id} + 1) - m_host_row_map(size_t{item_id}));
+    return m_values[m_host_row_map(size_t{item_id}) + i];
   }
 
   PUGS_INLINE
@@ -125,19 +121,13 @@ class SubItemValuePerItem
 
   // Following Kokkos logic, these classes are view and const view does allow
   // changes in data
-  PUGS_FORCEINLINE
-  DataType& operator[](size_t i) const noexcept(NO_ASSERT)
-  {
-    Assert(this->isBuilt());
-    return m_values[i];
-  }
-
-  template <typename IndexType>
-  DataType& operator[](const IndexType& i) const noexcept(NO_ASSERT)
+  template <typename ArrayIndexType>
+  DataType&
+  operator[](const ArrayIndexType& i) const noexcept(NO_ASSERT)
   {
-    static_assert(std::is_same_v<IndexType, size_t>, "Access to SubItemValuePerItem's array must be indexed by "
-                                                     "size_t");
+    static_assert(std::is_integral_v<ArrayIndexType>, "index must be an integral type");
     Assert(this->isBuilt());
+    Assert(i < m_values.size());
     return m_values[i];
   }
 
@@ -150,34 +140,40 @@ class SubItemValuePerItem
     return m_host_row_map.extent(0);
   }
 
-  PUGS_INLINE
-  size_t
-  numberOfSubValues(size_t i_cell) const noexcept(NO_ASSERT)
+  template <typename IndexType>
+  PUGS_INLINE size_t
+  numberOfSubValues(IndexType item_id) const noexcept(NO_ASSERT)
   {
+    static_assert(std::is_same_v<IndexType, ItemId>, "index must be an ItemId");
     Assert(this->isBuilt());
-    return m_host_row_map(i_cell + 1) - m_host_row_map(i_cell);
+    Assert(item_id < m_host_row_map.extent(0));
+    return m_host_row_map(size_t{item_id} + 1) - m_host_row_map(size_t{item_id});
   }
 
-  PUGS_INLINE
-  SubView
-  itemValues(size_t i_cell) noexcept(NO_ASSERT)
+  template <typename IndexType>
+  PUGS_INLINE SubView
+  itemValues(IndexType item_id) noexcept(NO_ASSERT)
   {
+    static_assert(std::is_same_v<IndexType, ItemId>, "index must be an ItemId");
     Assert(this->isBuilt());
-    const auto& cell_begin = m_host_row_map(i_cell);
-    const auto& cell_end   = m_host_row_map(i_cell + 1);
-    return SubView(m_values, cell_begin, cell_end);
+    Assert(item_id < m_host_row_map.extent(0));
+    const auto& item_begin = m_host_row_map(size_t{item_id});
+    const auto& item_end   = m_host_row_map(size_t{item_id} + 1);
+    return SubView(m_values, item_begin, item_end);
   }
 
   // Following Kokkos logic, these classes are view and const view does allow
   // changes in data
-  PUGS_INLINE
-  SubView
-  itemValues(size_t i_cell) const noexcept(NO_ASSERT)
+  template <typename IndexType>
+  PUGS_INLINE SubView
+  itemValues(IndexType item_id) const noexcept(NO_ASSERT)
   {
+    static_assert(std::is_same_v<IndexType, ItemId>, "index must be an ItemId");
     Assert(this->isBuilt());
-    const auto& cell_begin = m_host_row_map(i_cell);
-    const auto& cell_end   = m_host_row_map(i_cell + 1);
-    return SubView(m_values, cell_begin, cell_end);
+    Assert(item_id < m_host_row_map.extent(0));
+    const auto& item_begin = m_host_row_map(size_t{item_id});
+    const auto& item_end   = m_host_row_map(size_t{item_id} + 1);
+    return SubView(m_values, item_begin, item_end);
   }
 
   template <typename DataType2, typename ConnectivityPtr2>
-- 
GitLab