Clean up the ROOT readers and writers (#777)

* Clean up the ROOT readers and writers

- Add const where possible
- Use functions from `std::ranges` when possible
- Do not use `new` and `delete` when it's not needed (`ROOTReader.cc`)
- Clean up includes
- Change some indexes to size_t instead of int to avoid implicit conversions (doesn't change anything)

* Fix gcc13

* Fix pre-commit

* Fix pre-commit

* Add missing &

Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>

* Remove unnecessary check for m_fileVersion and add a few * after auto

---------

Co-authored-by: jmcarcell <jmcarcell@users.noreply.github.com>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>

Author: jmcarcell

include/podio/RNTupleReader.h

@@ -2,7 +2,6 @@
 #define PODIO_RNTUPLEREADER_H
 
 #include "podio/ROOTFrameData.h"
-#include "podio/SchemaEvolution.h"
 #include "podio/podioVersion.h"
 #include "podio/utilities/DatamodelRegistryIOHelpers.h"
 #include "podio/utilities/RootHelpers.h"
@@ -150,10 +149,11 @@ class RNTupleReader {
   /**
    * Read and reconstruct the generic parameters of the Frame
    */
-  GenericParameters readEventMetaData(const std::string& name, unsigned localEntry, unsigned readerIndex);
+  GenericParameters readEventMetaData(const std::string& name, const unsigned localEntry, const unsigned readerIndex);
 
   template <typename T>
-  void readParams(const std::string& name, unsigned entNum, unsigned readerIndex, GenericParameters& params);
+  void readParams(const std::string& name, const unsigned entNum, const unsigned readerIndex,
+                  GenericParameters& params);
 
   std::unique_ptr<root_compat::RNTupleReader> m_metadata{};
 

include/podio/RNTupleWriter.h

@@ -2,7 +2,6 @@
 #define PODIO_RNTUPLEWRITER_H
 
 #include "podio/Frame.h"
-#include "podio/SchemaEvolution.h"
 #include "podio/utilities/DatamodelRegistryIOHelpers.h"
 #include "podio/utilities/RootHelpers.h"
 

include/podio/ROOTReader.h

@@ -9,6 +9,7 @@
 #include "TChain.h"
 
 #include <memory>
+#include <optional>
 #include <string>
 #include <string_view>
 #include <tuple>

include/podio/ROOTWriter.h

@@ -1,7 +1,6 @@
 #ifndef PODIO_ROOTWRITER_H
 #define PODIO_ROOTWRITER_H
 
-#include "podio/CollectionIDTable.h"
 #include "podio/utilities/DatamodelRegistryIOHelpers.h"
 #include "podio/utilities/RootHelpers.h"
 

src/RNTupleReader.cc

@@ -1,7 +1,6 @@
 #include "podio/RNTupleReader.h"
 #include "podio/CollectionBufferFactory.h"
 #include "podio/CollectionBuffers.h"
-#include "podio/CollectionIDTable.h"
 #include "podio/DatamodelRegistry.h"
 #include "podio/GenericParameters.h"
 #include "podio/utilities/RootHelpers.h"
@@ -11,7 +10,11 @@
 
 #include <algorithm>
 #include <cstdint>
+#include <iostream>
 #include <memory>
+#include <string>
+#include <tuple>
+#include <vector>
 
 // Adjust for the move of this out of ROOT v7 in
 // https://github.com/root-project/root/pull/17281
@@ -24,15 +27,16 @@ using ROOT::Experimental::RException;
 namespace podio {
 
 template <typename T>
-void RNTupleReader::readParams(const std::string& name, unsigned localEntry, unsigned readerIndex,
+void RNTupleReader::readParams(const std::string& name, const unsigned localEntry, const unsigned readerIndex,
                                GenericParameters& params) {
   auto keyView = m_readers[name][readerIndex]->GetView<std::vector<std::string>>(root_utils::getGPKeyName<T>());
   auto valueView = m_readers[name][readerIndex]->GetView<std::vector<std::vector<T>>>(root_utils::getGPValueName<T>());
 
   params.loadFrom(keyView(localEntry), valueView(localEntry));
 }
 
-GenericParameters RNTupleReader::readEventMetaData(const std::string& name, unsigned localEntry, unsigned readerIndex) {
+GenericParameters RNTupleReader::readEventMetaData(const std::string& name, const unsigned localEntry,
+                                                   const unsigned readerIndex) {
   GenericParameters params;
 
   readParams<int>(name, localEntry, readerIndex, params);
@@ -44,11 +48,11 @@ GenericParameters RNTupleReader::readEventMetaData(const std::string& name, unsi
 }
 
 bool RNTupleReader::initCategory(const std::string& category) {
-  if (std::find(m_availableCategories.begin(), m_availableCategories.end(), category) == m_availableCategories.end()) {
+  if (std::ranges::find(m_availableCategories, category) == m_availableCategories.end()) {
     return false;
   }
   // Assume that the metadata is the same in all files
-  auto filename = m_filenames[0];
+  const auto& filename = m_filenames[0];
 
   auto collInfo = m_metadata_readers[filename]->GetView<std::vector<root_utils::CollectionWriteInfo>>(
       {root_utils::collInfoName(category)});
@@ -66,7 +70,7 @@ void RNTupleReader::openFile(const std::string& filename) {
 void RNTupleReader::openFiles(const std::vector<std::string>& filenames) {
 
   m_filenames.insert(m_filenames.end(), filenames.begin(), filenames.end());
-  for (auto& filename : filenames) {
+  for (const auto& filename : filenames) {
     if (m_metadata_readers.find(filename) == m_metadata_readers.end()) {
       m_metadata_readers[filename] = root_compat::RNTupleReader::Open(root_utils::metaTreeName, filename);
     }
@@ -75,7 +79,7 @@ void RNTupleReader::openFiles(const std::vector<std::string>& filenames) {
   m_metadata = root_compat::RNTupleReader::Open(root_utils::metaTreeName, filenames[0]);
 
   auto versionView = m_metadata->GetView<std::vector<uint16_t>>(root_utils::versionBranchName);
-  auto version = versionView(0);
+  const auto version = versionView(0);
 
   m_fileVersion = podio::version::Version{version[0], version[1], version[2]};
 
@@ -85,7 +89,7 @@ void RNTupleReader::openFiles(const std::vector<std::string>& filenames) {
   for (const auto& [name, _] : edm) {
     try {
       auto edmVersionView = m_metadata->GetView<std::vector<uint16_t>>(root_utils::edmVersionBranchName(name));
-      auto edmVersion = edmVersionView(0);
+      const auto edmVersion = edmVersionView(0);
       edmVersions.emplace_back(name, podio::version::Version{edmVersion[0], edmVersion[1], edmVersion[2]});
     } catch (const RException&) {
     }
@@ -100,7 +104,7 @@ unsigned RNTupleReader::getEntries(const std::string& name) {
   if (m_readers.find(name) == m_readers.end()) {
     m_readerEntries[name].reserve(m_filenames.size() + 1);
     m_readerEntries[name].push_back(0);
-    for (auto& filename : m_filenames) {
+    for (const auto& filename : m_filenames) {
       try {
         m_readers[name].emplace_back(root_compat::RNTupleReader::Open(name, filename));
         m_readerEntries[name].push_back(m_readerEntries[name].back() + m_readers[name].back()->GetNEntries());
@@ -159,25 +163,25 @@ std::unique_ptr<ROOTFrameData> RNTupleReader::readEntry(const std::string& categ
   // m_readerEntries contains the accumulated entries for all the readers
   // therefore, the first number that is lower or equal to the entry number
   // is at the index of the reader that contains the entry
-  auto upper = std::ranges::upper_bound(m_readerEntries[category], entNum);
-  auto localEntry = entNum - *(upper - 1);
-  auto readerIndex = upper - 1 - m_readerEntries[category].begin();
+  const auto upper = std::ranges::upper_bound(m_readerEntries[category], entNum);
+  const auto localEntry = entNum - *(upper - 1);
+  const auto readerIndex = upper - 1 - m_readerEntries[category].begin();
 
   ROOTFrameData::BufferMap buffers;
   // We need to create a non-bare entry here, because the entries for the
   // parameters are not explicitly (re)set and we need them default initialized.
   // In principle we would only need a bare entry for the collection data, since
   // we set all the fields there in any case.
-  auto dentry = m_readers[category][readerIndex]->GetModel().CreateEntry();
+  const auto dentry = m_readers[category][readerIndex]->GetModel().CreateEntry();
 
   for (const auto& coll : collInfo) {
     if (!collsToRead.empty() && std::ranges::find(collsToRead, coll.name) == collsToRead.end()) {
       continue;
     }
     const auto& collType = coll.dataType;
     const auto& bufferFactory = podio::CollectionBufferFactory::instance();
-    auto maybeBuffers = bufferFactory.createBuffers(collType, coll.schemaVersion, coll.isSubset);
-    auto collBuffers = maybeBuffers.value_or(podio::CollectionReadBuffers{});
+    const auto maybeBuffers = bufferFactory.createBuffers(collType, coll.schemaVersion, coll.isSubset);
+    const auto collBuffers = maybeBuffers.value_or(podio::CollectionReadBuffers{});
 
     if (!maybeBuffers) {
       std::cout << "WARNING: Buffers couldn't be created for collection " << coll.name << " of type " << coll.dataType
@@ -186,8 +190,8 @@ std::unique_ptr<ROOTFrameData> RNTupleReader::readEntry(const std::string& categ
     }
 
     if (coll.isSubset) {
-      auto brName = root_utils::subsetBranch(coll.name);
-      auto vec = new std::vector<podio::ObjectID>;
+      const auto brName = root_utils::subsetBranch(coll.name);
+      const auto vec = new std::vector<podio::ObjectID>;
       dentry->BindRawPtr(brName, vec);
       collBuffers.references->at(0) = std::unique_ptr<std::vector<podio::ObjectID>>(vec);
     } else {
@@ -196,7 +200,7 @@ std::unique_ptr<ROOTFrameData> RNTupleReader::readEntry(const std::string& categ
       const auto relVecNames = podio::DatamodelRegistry::instance().getRelationNames(collType);
       for (size_t j = 0; j < relVecNames.relations.size(); ++j) {
         const auto relName = relVecNames.relations[j];
-        auto vec = new std::vector<podio::ObjectID>;
+        const auto vec = new std::vector<podio::ObjectID>;
         const auto brName = root_utils::refBranch(coll.name, relName);
         dentry->BindRawPtr(brName, vec);
         collBuffers.references->at(j) = std::unique_ptr<std::vector<podio::ObjectID>>(vec);

src/RNTupleWriter.cc

@@ -1,6 +1,5 @@
 #include "podio/RNTupleWriter.h"
 #include "podio/DatamodelRegistry.h"
-#include "podio/SchemaEvolution.h"
 #include "podio/podioVersion.h"
 #include "podio/utilities/RootHelpers.h"
 #include "rootUtils.h"
@@ -67,7 +66,7 @@ void RNTupleWriter::writeFrame(const podio::Frame& frame, const std::string& cat
 
   // Use the writer as proxy to check whether this category has been initialized
   // already and do so if not
-  const bool new_category = (catInfo.writer == nullptr);
+  const bool new_category = catInfo.writer == nullptr;
   if (new_category) {
     // This is the minimal information that we need for now
     catInfo.names = podio::utils::sortAlphabeticaly(collsToWrite);
@@ -78,7 +77,7 @@ void RNTupleWriter::writeFrame(const podio::Frame& frame, const std::string& cat
   // Only loop over the collections that were requested in the first Frame of
   // this category
   for (const auto& name : catInfo.names) {
-    auto* coll = frame.getCollectionForWrite(name);
+    const auto* coll = frame.getCollectionForWrite(name);
     if (!coll) {
       // Make sure all collections that we want to write are actually available
       // NOLINTNEXTLINE(performance-inefficient-string-concatenation)
@@ -105,37 +104,37 @@ void RNTupleWriter::writeFrame(const podio::Frame& frame, const std::string& cat
     }
   }
 
-  auto entry = m_categories[category].writer->GetModel().CreateBareEntry();
+  const auto entry = m_categories[category].writer->GetModel().CreateBareEntry();
 
   RNTupleWriteOptions options;
   options.SetCompression(ROOT::RCompressionSetting::EDefaults::kUseGeneralPurpose);
 
   for (const auto& [name, coll] : collections) {
-    auto collBuffers = coll->getBuffers();
+    const auto collBuffers = coll->getBuffers();
     if (collBuffers.vecPtr) {
       entry->BindRawPtr(name, static_cast<void*>(collBuffers.vecPtr));
     }
 
     if (coll->isSubsetCollection()) {
-      auto& refColl = (*collBuffers.references)[0];
+      const auto& refColl = (*collBuffers.references)[0];
       const auto brName = root_utils::subsetBranch(name);
       entry->BindRawPtr(brName, refColl.get());
 
     } else {
 
       const auto relVecNames = podio::DatamodelRegistry::instance().getRelationNames(coll->getValueTypeName());
-      if (auto refColls = collBuffers.references) {
-        int i = 0;
-        for (auto& c : (*refColls)) {
+      if (const auto refColls = collBuffers.references) {
+        size_t i = 0;
+        for (const auto& c : *refColls) {
           const auto brName = root_utils::refBranch(name, relVecNames.relations[i]);
           entry->BindRawPtr(brName, c.get());
           ++i;
         }
       }
 
-      if (auto vmInfo = collBuffers.vectorMembers) {
-        int i = 0;
-        for (auto& [type, vec] : (*vmInfo)) {
+      if (const auto vmInfo = collBuffers.vectorMembers) {
+        size_t i = 0;
+        for (const auto& [type, vec] : *vmInfo) {
           const auto typeName = "vector<" + type + ">";
           const auto brName = root_utils::vecBranch(name, relVecNames.vectorMembers[i]);
           auto ptr = *static_cast<std::vector<int>**>(vec);
@@ -163,41 +162,41 @@ std::unique_ptr<root_compat::RNTupleModel>
 RNTupleWriter::createModels(const std::vector<root_utils::StoreCollection>& collections) {
   auto model = root_compat::RNTupleModel::CreateBare();
 
-  for (auto& [name, coll] : collections) {
+  for (const auto& [name, coll] : collections) {
     // For the first entry in each category we also record the datamodel
     // definition
     m_datamodelCollector.registerDatamodelDefinition(coll, name);
 
     const auto collBuffers = coll->getBuffers();
 
     if (collBuffers.vecPtr) {
-      auto collClassName = "std::vector<" + std::string(coll->getDataTypeName()) + ">";
+      const auto collClassName = "std::vector<" + std::string(coll->getDataTypeName()) + ">";
       auto field = RFieldBase::Create(name, collClassName).Unwrap();
       model->AddField(std::move(field));
     }
 
     if (coll->isSubsetCollection()) {
       const auto brName = root_utils::subsetBranch(name);
-      auto collClassName = "vector<podio::ObjectID>";
+      const auto collClassName = "vector<podio::ObjectID>";
       auto field = RFieldBase::Create(brName, collClassName).Unwrap();
       model->AddField(std::move(field));
     } else {
 
       const auto relVecNames = podio::DatamodelRegistry::instance().getRelationNames(coll->getValueTypeName());
-      if (auto refColls = collBuffers.references) {
-        int i = 0;
-        for (auto& c [[maybe_unused]] : (*refColls)) {
+      if (const auto refColls = collBuffers.references) {
+        size_t i = 0;
+        for (const auto& c [[maybe_unused]] : *refColls) {
           const auto brName = root_utils::refBranch(name, relVecNames.relations[i]);
-          auto collClassName = "vector<podio::ObjectID>";
+          const auto collClassName = "vector<podio::ObjectID>";
           auto field = RFieldBase::Create(brName, collClassName).Unwrap();
           model->AddField(std::move(field));
           ++i;
         }
       }
 
-      if (auto vminfo = collBuffers.vectorMembers) {
-        int i = 0;
-        for (auto& [type, vec] : (*vminfo)) {
+      if (const auto vminfo = collBuffers.vectorMembers) {
+        size_t i = 0;
+        for (const auto& [type, vec] : *vminfo) {
           const auto typeName = "vector<" + type + ">";
           const auto brName = root_utils::vecBranch(name, relVecNames.vectorMembers[i]);
           auto field = RFieldBase::Create(brName, typeName).Unwrap();
@@ -227,46 +226,48 @@ RNTupleWriter::createModels(const std::vector<root_utils::StoreCollection>& coll
 }
 
 RNTupleWriter::CategoryInfo& RNTupleWriter::getCategoryInfo(const std::string& category) {
-  if (auto it = m_categories.find(category); it != m_categories.end()) {
+  if (const auto it = m_categories.find(category); it != m_categories.end()) {
     return it->second;
   }
 
-  auto [it, _] = m_categories.try_emplace(category, CategoryInfo{});
+  const auto [it, _] = m_categories.try_emplace(category, CategoryInfo{});
   return it->second;
 }
 
 void RNTupleWriter::finish() {
   auto metadata = root_compat::RNTupleModel::Create();
 
-  auto podioVersion = podio::version::build_version;
+  const auto podioVersion = podio::version::build_version;
   auto versionField = metadata->MakeField<std::vector<uint16_t>>(root_utils::versionBranchName);
   *versionField = {podioVersion.major, podioVersion.minor, podioVersion.patch};
 
-  auto edmDefinitions = m_datamodelCollector.getDatamodelDefinitionsToWrite();
+  const auto edmDefinitions = m_datamodelCollector.getDatamodelDefinitionsToWrite();
   for (const auto& [name, _] : edmDefinitions) {
-    auto edmVersion = DatamodelRegistry::instance().getDatamodelVersion(name);
+    const auto edmVersion = DatamodelRegistry::instance().getDatamodelVersion(name);
     if (edmVersion) {
-      auto edmVersionField = metadata->MakeField<std::vector<uint16_t>>(root_utils::edmVersionBranchName(name).c_str());
+      const auto edmVersionField = metadata->MakeField<std::vector<uint16_t>>(root_utils::edmVersionBranchName(name));
       *edmVersionField = {edmVersion->major, edmVersion->minor, edmVersion->patch};
     }
   }
 
-  auto edmField = metadata->MakeField<std::vector<std::tuple<std::string, std::string>>>(root_utils::edmDefBranchName);
+  const auto edmField =
+      metadata->MakeField<std::vector<std::tuple<std::string, std::string>>>(root_utils::edmDefBranchName);
   *edmField = std::move(edmDefinitions);
 
-  auto availableCategoriesField = metadata->MakeField<std::vector<std::string>>(root_utils::availableCategories);
-  for (auto& [c, _] : m_categories) {
+  const auto availableCategoriesField = metadata->MakeField<std::vector<std::string>>(root_utils::availableCategories);
+  for (const auto& [c, _] : m_categories) {
     availableCategoriesField->push_back(c);
   }
 
-  for (auto& [category, collInfo] : m_categories) {
-    auto collInfoField =
+  for (const auto& [category, collInfo] : m_categories) {
+    const auto collInfoField =
         metadata->MakeField<std::vector<root_utils::CollectionWriteInfo>>({root_utils::collInfoName(category)});
     *collInfoField = collInfo.collInfo;
   }
 
   metadata->Freeze();
-  auto metadataWriter = root_compat::RNTupleWriter::Append(std::move(metadata), root_utils::metaTreeName, *m_file, {});
+  const auto metadataWriter =
+      root_compat::RNTupleWriter::Append(std::move(metadata), root_utils::metaTreeName, *m_file, {});
 
   metadataWriter->Fill();