Merge pull request #1261 from LLNL/feature/kweiss/sidre-attributes

Bugfix when loading group with attributes

RELEASE-NOTES.md

@@ -59,6 +59,7 @@ The Axom project release numbers follow [Semantic Versioning](http://semver.org/
 - primal's `Polygon` area computation in 3D previously only worked when the polygon was aligned with the XY-plane. It now works for arbitrary polygons.
 - Upgrades our `vcpkg` usage for automated Windows builds of our TPLs to its [2023.12.12 release](https://github.com/microsoft/vcpkg/releases/tag/2023.12.12)
 - Fixed a bug in the bounds checks for `primal::clip(Triangle, BoundingBox)`
+- Fixed a bug when loading Sidre groups with attributes that already exist
 
 ## [Version 0.8.1] - Release date 2023-08-16
 

src/axom/sidre/core/AttrValues.cpp

@@ -35,33 +35,21 @@ namespace sidre
  */
 bool AttrValues::hasValue(const Attribute* attr) const
 {
-  if(attr == nullptr)
+  // check for empty values
+  if(attr == nullptr || m_values == nullptr)
   {
     return false;
   }
 
-  if(m_values == nullptr)
-  {
-    // No attributes have been set in this View.
-    return false;
-  }
-
-  IndexType iattr = attr->getIndex();
-
-  if((size_t)iattr >= m_values->size())
+  const auto iattr = static_cast<std::size_t>(attr->getIndex());
+  if(iattr >= m_values->size())
   {
     // This attribute has not been set for this View.
     return false;
   }
 
   Node& value = (*m_values)[iattr];
-
-  if(isEmpty(value))
-  {
-    return false;
-  }
-
-  return true;
+  return !isEmpty(value);
 }
 
 /*
@@ -87,9 +75,8 @@ bool AttrValues::setToDefault(const Attribute* attr)
     return true;
   }
 
-  IndexType iattr = attr->getIndex();
-
-  if((size_t)iattr >= m_values->size())
+  const auto iattr = static_cast<std::size_t>(attr->getIndex());
+  if(iattr >= m_values->size())
   {
     // This attribute has not been set for this View, already default.
     return true;
@@ -117,13 +104,13 @@ bool AttrValues::createNode(IndexType iattr)
     m_values = new(std::nothrow) Values();
   }
 
-  if((size_t)iattr >= m_values->size())
+  if(static_cast<std::size_t>(iattr) >= m_values->size())
   {
     // Create all attributes up to iattr, push back empty Nodes
     m_values->reserve(iattr + 1);
     for(int n = m_values->size(); n < iattr + 1; ++n)
     {
-      m_values->push_back(Node());
+      m_values->emplace_back(Node());
     }
   }
 
@@ -192,9 +179,8 @@ const Node& AttrValues::getValueNodeRef(const Attribute* attr) const
     return attr->getDefaultNodeRef();
   }
 
-  IndexType iattr = attr->getIndex();
-
-  if((size_t)iattr >= m_values->size())
+  const auto iattr = static_cast<std::size_t>(attr->getIndex());
+  if(iattr >= m_values->size())
   {
     // This attribute has not been set for this View
     return attr->getDefaultNodeRef();

src/axom/sidre/core/AttrValues.hpp

@@ -58,16 +58,12 @@ class View;
  * avoid multiple messages for the same error.  For example, if the
  * index cannot be converted to an Attribute pointer in the View
  * class, an error message will be printing and then a NULL pointer
- * passed to the AttrValues class which will not print another
- * message.
+ * passed to the AttrValues class which will not print another message.
  */
 class AttrValues
 {
 public:
-  /*!
-   * Friend declarations to constrain usage via controlled access to
-   * private members.
-   */
+  /// Friend declarations to constrain usage via controlled access to private members.
   friend class View;
 
 private:
@@ -106,7 +102,7 @@ class AttrValues
   template <typename ScalarType>
   bool setScalar(const Attribute* attr, ScalarType value)
   {
-    DataTypeId arg_id = detail::SidreTT<ScalarType>::id;
+    const DataTypeId arg_id = detail::SidreTT<ScalarType>::id;
     if(arg_id != attr->getTypeID())
     {
       SLIC_CHECK_MSG(arg_id == attr->getTypeID(),
@@ -117,8 +113,8 @@ class AttrValues
       return false;
     }
 
-    IndexType iattr = attr->getIndex();
-    bool ok = createNode(iattr);
+    const IndexType iattr = attr->getIndex();
+    const bool ok = createNode(iattr);
     if(ok)
     {
       (*m_values)[iattr] = value;
@@ -131,7 +127,7 @@ class AttrValues
    */
   bool setString(const Attribute* attr, const std::string& value)
   {
-    DataTypeId arg_id = CHAR8_STR_ID;
+    const DataTypeId arg_id = CHAR8_STR_ID;
     if(arg_id != attr->getTypeID())
     {
       SLIC_CHECK_MSG(arg_id == attr->getTypeID(),
@@ -142,8 +138,8 @@ class AttrValues
       return false;
     }
 
-    IndexType iattr = attr->getIndex();
-    bool ok = createNode(iattr);
+    const IndexType iattr = attr->getIndex();
+    const bool ok = createNode(iattr);
     if(ok)
     {
       (*m_values)[iattr] = value;
@@ -159,8 +155,8 @@ class AttrValues
    */
   bool setNode(const Attribute* attr, const Node& node)
   {
-    IndexType iattr = attr->getIndex();
-    bool ok = createNode(iattr);
+    const IndexType iattr = attr->getIndex();
+    const bool ok = createNode(iattr);
     if(ok)
     {
       (*m_values)[iattr] = node;

src/axom/sidre/core/DataStore.cpp

@@ -637,14 +637,9 @@ bool DataStore::saveAttributeLayout(Node& node) const
 
   bool hasAttributes = false;
 
-  IndexType aidx = getFirstValidAttributeIndex();
-  while(indexIsValid(aidx))
+  for(const auto& attr : attributes())
   {
-    const Attribute* attr = getAttribute(aidx);
-
-    node[attr->getName()] = attr->getDefaultNodeRef();
-
-    aidx = getNextValidAttributeIndex(aidx);
+    node[attr.getName()] = attr.getDefaultNodeRef();
     hasAttributes = true;
   }
 
@@ -670,7 +665,9 @@ void DataStore::loadAttributeLayout(Node& node)
       Node& n_attr = attrs_itr.next();
       std::string attr_name = attrs_itr.name();
 
-      Attribute* attr = createAttributeEmpty(attr_name);
+      auto* attr = !hasAttribute(attr_name) ? createAttributeEmpty(attr_name)
+                                            : getAttribute(attr_name);
+
       attr->setDefaultNodeRef(n_attr);
     }
   }

src/axom/sidre/core/DataStore.hpp

@@ -279,7 +279,7 @@ class DataStore
   IndexType getNumAttributes() const;
 
   /*!
-   * \brief Create a Attribute object with a default scalar value.
+   * \brief Create an Attribute object with a default scalar value.
    *
    *        The Attribute object is assigned a unique index when created and the
    *        Attribute object is owned by the DataStore object.
@@ -297,7 +297,7 @@ class DataStore
   }
 
   /*!
-   * \brief Create a Attribute object with a default string value.
+   * \brief Create an Attribute object with a default string value.
    *
    *        The Attribute object is assigned a unique index when created and the
    *        Attribute object is owned by the DataStore object.
@@ -313,20 +313,14 @@ class DataStore
     return new_attribute;
   }
 
-  /*!
-   * \brief Return true if DataStore has created attribute name; else false.
-   */
+  /// \brief Return true if DataStore has created attribute name, else false
   bool hasAttribute(const std::string& name) const;
 
-  /*!
-   * \brief Return true if DataStore has created attribute with index; else
-   * false.
-   */
+  /// \brief Return true if DataStore has created attribute with index, else false
   bool hasAttribute(IndexType idx) const;
 
   /*!
-   * \brief Remove Attribute from the DataStore and destroy it and
-   *        its data.
+   * \brief Remove Attribute from the DataStore and destroy it and its data.
    *
    * \note Destruction of an Attribute detaches it from all Views to
    *       which it is attached.
@@ -343,17 +337,15 @@ class DataStore
   void destroyAttribute(IndexType idx);
 
   /*!
-   * \brief Remove Attribute from the DataStore and destroy it and
-   *        its data.
+   * \brief Remove Attribute from the DataStore and destroy it and its data.
    *
    * \note Destruction of an Attribute detaches it from all Views to
    *       which it is attached.
    */
   void destroyAttribute(Attribute* attr);
 
   /*!
-   * \brief Remove all Attributes from the DataStore and destroy them
-   *        and their data.
+   * \brief Remove all Attributes from the DataStore and destroy them and their data.
    *
    * \note Destruction of an Attribute detaches it from all Views to
    *       which it is attached.
@@ -399,9 +391,7 @@ class DataStore
    */
   bool saveAttributeLayout(Node& node) const;
 
-  /*!
-   * \brief Create attributes from name/value pairs in node["attribute"].
-   */
+  /// \brief Create attributes from name/value pairs in node["attribute"].
   void loadAttributeLayout(Node& node);
 
   //@}
@@ -434,8 +424,7 @@ class DataStore
 
   /*!
    * \brief Return next valid Attribute index in DataStore object after given
-   * index (i.e., smallest index over all Attribute indices larger than given
-   * one).
+   * index (i.e., smallest index over all Attribute indices larger than given one).
    *
    * sidre::InvalidIndex is returned if there is no valid index greater
    * than given one.