Code cleanup and in-code documentation

Author: franzpoeschel

include/openPMD/IO/ADIOS/ADIOS2IOHandler.hpp

@@ -220,6 +220,10 @@ class ADIOS2IOHandlerImpl
 
 private:
     adios2::ADIOS m_ADIOS;
+    /*
+     * If the iteration encoding is variableBased, we default to using the
+     * 2021_02_09 schema since it allows mutable attributes.
+     */
     IterationEncoding m_iterationEncoding = IterationEncoding::groupBased;
     /**
      * The ADIOS2 engine type, to be passed to adios2::IO::SetEngine
@@ -1305,7 +1309,7 @@ class ADIOS2IOHandler : public AbstractIOHandler
 {
 #if openPMD_HAVE_ADIOS2
 
-    friend class ADIOS2IOHandlerImpl;
+friend class ADIOS2IOHandlerImpl;
 
 private:
     ADIOS2IOHandlerImpl m_impl;

include/openPMD/IO/IOTask.hpp

@@ -137,6 +137,11 @@ struct OPENPMDAPI_EXPORT Parameter< Operation::OPEN_FILE > : public AbstractPara
     }
 
     std::string name = "";
+    /*
+     * The backends might need to ensure availability of certain features
+     * for some iteration encodings, e.g. availability of ADIOS steps for
+     * variableBased encoding.
+     */
     IterationEncoding encoding = IterationEncoding::groupBased;
 };
 

include/openPMD/Iteration.hpp

@@ -156,9 +156,25 @@ class Iteration : public LegacyAttributable
 
     struct DeferredParseAccess
     {
+        /**
+         * The group path within /data containing this iteration.
+         * Example: "1" for iteration 1, "" in variable-based iteration
+         * encoding.
+         */
         std::string path;
+        /**
+         * The iteration index as accessed by the user in series.iterations[i]
+         */
         uint64_t iteration = 0;
+        /**
+         * If this iteration is part of a Series with file-based layout.
+         * (Group- and variable-based parsing shares the same code logic.)
+         */
         bool fileBased = false;
+        /**
+         * If fileBased == true, the file name (without file path) of the file
+         * containing this iteration.
+         */
         std::string filename;
     };
 
@@ -179,11 +195,15 @@ class Iteration : public LegacyAttributable
      * allow for those different control flows.
      * Finally, read_impl() is called which contains the common parsing
      * logic for an iteration.
+     * 
+     * reread() reads again an Iteration that has been previously read.
+     * Calling it on an Iteration not yet parsed is an error.
      *
      */
-    void read( std::string path, bool reread = false );
+    void read();
+    void reread( std::string const & path );
     void readFileBased( std::string filePath, std::string const & groupPath );
-    void readGroupBased( std::string const & groupPath );
+    void readGorVBased( std::string const & groupPath );
     void read_impl( std::string const & groupPath );
 
     /**

include/openPMD/backend/Container.hpp

@@ -292,6 +292,14 @@ class Container : public LegacyAttributable
 
     std::shared_ptr< InternalContainer > m_container;
 
+    /**
+     * This class wraps a Container and forwards operator[]() and at() to it.
+     * It remembers the keys used for accessing. Upon going out of scope, all
+     * keys not yet accessed are removed from the Container.
+     * Note that the container is stored by non-owning reference, thus
+     * requiring that the original Container stay in scope while using this
+     * class.
+     */
     class EraseStaleEntries
     {
         std::set< key_type > m_accessedKeys;

src/Iteration.cpp

@@ -324,13 +324,8 @@ void Iteration::deferParseAccess( DeferredParseAccess dr )
         auxiliary::makeOption< DeferredParseAccess >( std::move( dr ) );
 }
 
-void Iteration::read( std::string path, bool reread )
+void Iteration::read()
 {
-    if( reread )
-    {
-        read_impl( path );
-        return;
-    }
     if( !m_deferredParseAccess->has_value() )
     {
         return;
@@ -342,12 +337,23 @@ void Iteration::read( std::string path, bool reread )
     }
     else
     {
-        readGroupBased( deferred.path );
+        readGorVBased( deferred.path );
     }
     // reset this thing
     *m_deferredParseAccess = auxiliary::Option< DeferredParseAccess >();
 }
 
+void Iteration::reread( std::string const & path )
+{
+    if( m_deferredParseAccess->has_value() )
+    {
+        throw std::runtime_error(
+            "[Iteration] Internal control flow error: Trying to reread an "
+            "iteration that has not yet been read for its first time." );
+    }
+    read_impl( path );
+}
+
 void Iteration::readFileBased(
     std::string filePath, std::string const & groupPath )
 {
@@ -358,7 +364,7 @@ void Iteration::readFileBased(
     read_impl( groupPath );
 }
 
-void Iteration::readGroupBased( std::string const & groupPath )
+void Iteration::readGorVBased( std::string const & groupPath )
 {
 
     read_impl(groupPath );
@@ -665,7 +671,7 @@ void Iteration::runDeferredParseAccess()
     *newAccess = Access::READ_WRITE;
     try
     {
-        read( "", false );
+        read();
     }
     catch( ... )
     {

src/Mesh.cpp

@@ -235,14 +235,6 @@ Mesh::flush_impl(std::string const& name)
 void
 Mesh::read()
 {
-    /*
-     * @todo This is a proof-of-concept on how we need to rework our re-parsing
-     * procedures. Goals when parsing an iteration again:
-     * 1. Old handles must survive
-     * 2. Map entries that cannot be found any more must perish.
-     * @todo This approach will kill references that users may have to mapped
-     * components, so improve that.
-     */
     auto map = eraseStaleEntries();
 
     using DT = Datatype;

src/Series.cpp

@@ -893,12 +893,15 @@ void SeriesImpl::readOneIterationFileBased( std::string const & filePath )
         }
         else if( encoding == "variableBased" )
         {
-            // @todo should we throw? test this path
-            series.m_iterationEncoding = IterationEncoding::variableBased;
-            std::cerr << "Series constructor called with iteration "
-                            "regex '%T' suggests loading a "
-                        << "time series with fileBased iteration "
-                            "encoding. Loaded file is variableBased.\n";
+            /*
+             * Unlike if the file were group-based, this one doesn't work
+             * at all since the group paths are different.
+             */
+            throw std::runtime_error(
+                "Series constructor called with iteration "
+                "regex '%T' suggests loading a "
+                "time series with fileBased iteration "
+                "encoding. Loaded file is variableBased." );
         }
         else
             throw std::runtime_error(
@@ -1026,7 +1029,7 @@ SeriesImpl::readGorVBased( bool do_init )
             {
                 pOpen.path = path;
                 IOHandler()->enqueue( IOTask( &i, pOpen ) );
-                i.read( path, /* reread = */ true );
+                i.reread( path );
             }
         }
         else