Add reviewer's suggestions

Also fix a bug in group-based mode

Author: franzpoeschel

include/openPMD/Iteration.hpp

@@ -116,7 +116,8 @@ class Iteration : public Attributable
      *        Background: Upon calling Iteration::close(), the openPMD API
      *        will add metadata to the iteration in form of an attribute,
      *        indicating that the iteration has indeed been closed.
-     *        Useful mainly in streaming context.
+     *        Useful mainly in streaming context when a reader inquires from
+     *        a writer that it is done writing.
      *
      * @return Whether the iteration has been explicitly closed (yet) by the
      *         writer.
@@ -136,11 +137,17 @@ class Iteration : public Attributable
     void flush();
     void read();
 
+    /**
+     * @brief Whether an iteration has been closed yet.
+     *
+     */
     enum class CloseStatus
     {
-        Open,
-        ClosedInFrontend,
-        ClosedInBackend
+        Open,             //!< Iteration has not been closed
+        ClosedInFrontend, /*!< Iteration has been closed, but task has not yet
+                               been propagated to the backend */
+        ClosedInBackend   /*!< Iteration has been closed and task has been
+                               propagated to the backend */
     };
 
     /*
@@ -152,14 +159,16 @@ class Iteration : public Attributable
     std::shared_ptr< CloseStatus > m_closed =
         std::make_shared< CloseStatus >( CloseStatus::Open );
 
-    /**
-     * @brief Verify that a closed iteration has not been wrongly accessed.
+    /*
+     * @brief Check recursively whether this Iteration is dirty.
+     *        It is dirty if any attribute or dataset is read from or written to
+     *        the backend.
      *
-     * @return true If closed iteration had no wrong accesses.
+     * @return true If dirty.
      * @return false Otherwise.
      */
     bool
-    verifyClosed() const;
+    dirtyRecursive() const;
 
     virtual void linkHierarchy(std::shared_ptr< Writable > const& w);
 };  // Iteration

include/openPMD/Mesh.hpp

@@ -182,14 +182,6 @@ class Mesh : public BaseRecord< MeshRecordComponent >
 
     void flush_impl(std::string const&) override;
     void read() override;
-
-    /**
-     * @brief Verify that a mesh in a closed iteration has not
-     *        been wrongly accessed.
-     *
-     * @return true If closed iteration had no wrong accesses.
-     * @return false Otherwise.
-     */
 }; // Mesh
 
 template< typename T >

include/openPMD/ParticleSpecies.hpp

@@ -47,14 +47,15 @@ class ParticleSpecies : public Container< Record >
     void flush(std::string const &) override;
 
     /**
-     * @brief Verify that a particle species in a closed iteration has not
-     *        been wrongly accessed.
+     * @brief Check recursively whether this ParticleSpecies is dirty.
+     *        It is dirty if any attribute or dataset is read from or written to
+     *        the backend.
      *
-     * @return true If closed iteration had no wrong accesses.
+     * @return true If dirty.
      * @return false Otherwise.
      */
     bool
-    verifyClosed() const;
+    dirtyRecursive() const;
 };
 
 namespace traits

include/openPMD/RecordComponent.hpp

@@ -200,14 +200,15 @@ class RecordComponent : public BaseRecordComponent
     RecordComponent& makeEmpty( Dataset d );
 
     /**
-     * @brief Verify that a record component in a closed iteration has not
-     *        been wrongly accessed.
+     * @brief Check recursively whether this RecordComponent is dirty.
+     *        It is dirty if any attribute or dataset is read from or written to
+     *        the backend.
      *
-     * @return true If closed iteration had no wrong accesses.
+     * @return true If dirty.
      * @return false Otherwise.
      */
     bool
-    verifyClosed() const;
+    dirtyRecursive() const;
 }; // RecordComponent
 
 

include/openPMD/backend/BaseRecord.hpp

@@ -99,14 +99,15 @@ class BaseRecord : public Container< T_elem >
     virtual void read() = 0;
 
     /**
-     * @brief Verify that a base record in a closed iteration has not
-     *        been wrongly accessed.
+     * @brief Check recursively whether this BaseRecord is dirty.
+     *        It is dirty if any attribute or dataset is read from or written to
+     *        the backend.
      *
-     * @return true If closed iteration had no wrong accesses.
+     * @return true If dirty.
      * @return false Otherwise.
      */
     bool
-    verifyClosed() const;
+    dirtyRecursive() const;
 }; // BaseRecord
 
 
@@ -308,19 +309,19 @@ BaseRecord< T_elem >::flush(std::string const& name)
 
 template< typename T_elem >
 inline bool
-BaseRecord< T_elem >::verifyClosed() const
+BaseRecord< T_elem >::dirtyRecursive() const
 {
     if( Attributable::dirty )
     {
-        return false;
+        return true;
     }
     for( auto const & pair : *this )
     {
-        if( !pair.second.verifyClosed() )
+        if( pair.second.dirtyRecursive() )
         {
-            return false;
+            return true;
         }
     }
-    return true;
+    return false;
 }
 } // namespace openPMD

src/IO/HDF5/HDF5IOHandler.cpp

@@ -406,15 +406,17 @@ HDF5IOHandlerImpl::closeFile(
     auto fileID_it = m_fileIDs.find( writable );
     if( fileID_it == m_fileIDs.end() )
     {
+        throw std::runtime_error(
+            "[HDF5] Trying to close a file that is not "
+            "present in the backend" );
         return;
     }
     hid_t fileID = fileID_it->second;
     H5Fclose( fileID );
     m_openFileIDs.erase( fileID );
     m_fileIDs.erase( fileID_it );
 
-    /*
-     * std::unordered_map::erase:
+    /* std::unordered_map::erase:
      * References and iterators to the erased elements are invalidated. Other
      * iterators and references are not invalidated.
      */

src/Iteration.cpp

@@ -20,14 +20,15 @@
  */
 #include "openPMD/Iteration.hpp"
 
-#include <tuple>
-
 #include "openPMD/Dataset.hpp"
 #include "openPMD/Datatype.hpp"
 #include "openPMD/Series.hpp"
+#include "openPMD/auxiliary/DerefDynamicCast.hpp"
 #include "openPMD/auxiliary/StringManip.hpp"
 #include "openPMD/backend/Writable.hpp"
 
+#include <tuple>
+
 
 namespace openPMD
 {
@@ -113,7 +114,7 @@ Iteration::close( bool _flush )
     *m_closed = CloseStatus::ClosedInFrontend;
     if( _flush )
     {
-        Series * s = dynamic_cast< Series * >(
+        Series * s = &auxiliary::deref_dynamic_cast< Series >(
             parent->attributable->parent->attributable );
         // figure out my iteration number
         uint64_t index;
@@ -431,27 +432,27 @@ Iteration::read()
 }
 
 bool
-Iteration::verifyClosed() const
+Iteration::dirtyRecursive() const
 {
     if( dirty )
     {
-        return false;
+        return true;
     }
     for( auto const & pair : particles )
     {
-        if( !pair.second.verifyClosed() )
+        if( pair.second.dirtyRecursive() )
         {
-            return false;
+            return true;
         }
     }
     for( auto const & pair : meshes )
     {
-        if( !pair.second.verifyClosed() )
+        if( pair.second.dirtyRecursive() )
         {
-            return false;
+            return true;
         }
     }
-    return true;
+    return false;
 }
 
 void

src/ParticleSpecies.cpp

@@ -145,19 +145,19 @@ ParticleSpecies::flush(std::string const& path)
 }
 
 bool
-ParticleSpecies::verifyClosed() const
+ParticleSpecies::dirtyRecursive() const
 {
     if( dirty )
     {
-        return false;
+        return true;
     }
     for( auto const & pair : *this )
     {
-        if( !pair.second.verifyClosed() )
+        if( pair.second.dirtyRecursive() )
         {
-            return false;
+            return true;
         }
     }
-    return true;
+    return false;
 }
 } // namespace openPMD

src/RecordComponent.cpp

@@ -267,12 +267,12 @@ RecordComponent::readBase()
 }
 
 bool
-RecordComponent::verifyClosed() const
+RecordComponent::dirtyRecursive() const
 {
     if( Attributable::dirty )
     {
-        return false;
+        return true;
     }
-    return m_chunks->empty();
+    return !m_chunks->empty();
 }
 } // namespace openPMD

src/Series.cpp

@@ -566,7 +566,7 @@ Series::flushFileBased( IterationsContainer && iterationsToFlush )
                 // file corresponding with the iteration has previously been
                 // closed and fully flushed
                 // verify that there have been no further accesses
-                if( !i.second.verifyClosed() )
+                if( i.second.dirtyRecursive() )
                 {
                     throw std::runtime_error(
                         "[Series] Detected illegal access to iteration that "
@@ -599,7 +599,7 @@ Series::flushFileBased( IterationsContainer && iterationsToFlush )
                         "[Series] Closed iteration has not been written. This "
                         "is an internal error.");
                 }
-                if( !i.second.verifyClosed() )
+                if( i.second.dirtyRecursive() )
                 {
                     throw std::runtime_error(
                         "[Series] Detected illegal access to iteration that "
@@ -657,7 +657,7 @@ Series::flushGroupBased( IterationsContainer && iterationsToFlush )
                 // file corresponding with the iteration has previously been
                 // closed and fully flushed
                 // verify that there have been no further accesses
-                if( !i.second.verifyClosed() )
+                if( i.second.dirtyRecursive() )
                 {
                     throw std::runtime_error(
                         "[Series] Illegal access to iteration " +
@@ -667,6 +667,12 @@ Series::flushGroupBased( IterationsContainer && iterationsToFlush )
                 continue;
             }
             i.second.flush();
+            if( *i.second.m_closed == Iteration::CloseStatus::ClosedInFrontend )
+            {
+                // the iteration has no dedicated file in group-based mode
+                *i.second.m_closed = Iteration::CloseStatus::ClosedInBackend;
+            }
+            IOHandler->flush();
         }
     else
     {
@@ -692,7 +698,7 @@ Series::flushGroupBased( IterationsContainer && iterationsToFlush )
                         "[Series] Closed iteration has not been written. This "
                         "is an internal error.");
                 }
-                if( !i.second.verifyClosed() )
+                if( i.second.dirtyRecursive() )
                 {
                     throw std::runtime_error(
                         "[Series] Illegal access to iteration " +
@@ -707,9 +713,15 @@ Series::flushGroupBased( IterationsContainer && iterationsToFlush )
                 i.second.parent = getWritable(&iterations);
             }
             i.second.flushGroupBased(i.first);
+            if( *i.second.m_closed == Iteration::CloseStatus::ClosedInFrontend )
+            {
+                // the iteration has no dedicated file in group-based mode
+                *i.second.m_closed = Iteration::CloseStatus::ClosedInBackend;
+            }
         }
 
         flushAttributes();
+        IOHandler->flush();
     }
 }