DPL: Allow passing labeled arrays through command line

Framework/Core/include/Framework/Array2D.h

@@ -79,19 +79,29 @@ struct Array2D {
 
   Array2D& operator=(Array2D<T> const& other)
   {
-    this->rows = other.rows;
-    this->cols = other.cols;
-    data = new T[rows * cols];
-    for (auto i = 0U; i < rows; ++i) {
-      for (auto j = 0U; j < cols; ++j) {
-        data[i * cols + j] = *(other.data + (i * cols + j));
+    if (this == &other) {
+      return *this;
+    }
+    // Copy into a new buffer first, so that a throwing copy leaves this array untouched
+    auto* newData = new T[other.rows * other.cols];
+    for (auto i = 0U; i < other.rows; ++i) {
+      for (auto j = 0U; j < other.cols; ++j) {
+        newData[i * other.cols + j] = other.data[i * other.cols + j];
       }
     }
+    delete[] data;
+    data = newData;
+    this->rows = other.rows;
+    this->cols = other.cols;
     return *this;
   }
 
   Array2D& operator=(Array2D<T>&& other)
   {
+    if (this == &other) {
+      return *this;
+    }
+    delete[] data;
     this->rows = other.rows;
     this->cols = other.cols;
     data = other.data;

Framework/Core/include/Framework/ConfigParamsHelper.h

@@ -37,6 +37,14 @@ struct ConfigParamsHelper {
                                options_description& options,
                                boost::program_options::options_description const& vetos = options_description());
 
+  /// Maximum length of a default value shown in the help, so that
+  /// an option and its description fit into a typical terminal line
+  static constexpr size_t MaxHelpDefaultLength = 40;
+
+  /// Text to show in the help for the default @a value: values longer than
+  /// MaxHelpDefaultLength are reduced to their beginning and end, separated by "..."
+  static std::string defaultValueForHelp(std::string const& value);
+
   /// Check if option is defined
   static bool hasOption(const std::vector<ConfigParamSpec>& specs, const std::string& optName);
 
@@ -112,7 +120,12 @@ struct ConfigParamsHelper {
       using Type = typename variant_type<V>::type;
       using BoostType = typename std::conditional<V == VariantType::String, std::string, Type>::type;
       auto value = boost::program_options::value<BoostType>();
-      value = value->default_value(spec.defaultValue.get<BoostType>());
+      if constexpr (V == VariantType::String) {
+        auto defaultValue = spec.defaultValue.get<BoostType>();
+        value = value->default_value(defaultValue, defaultValueForHelp(defaultValue));
+      } else {
+        value = value->default_value(spec.defaultValue.get<BoostType>());
+      }
       if constexpr (V == VariantType::Bool) {
         // for bool values we also support the zero_token option to make
         // the option usable as a single switch
@@ -121,11 +134,18 @@ struct ConfigParamsHelper {
       options.add_options()(name, value, help);
     } else if constexpr (isArray<V>() || isArray2D<V>()) {
       auto value = boost::program_options::value<std::string>();
-      value = value->default_value(spec.defaultValue.asString());
+      auto defaultValue = spec.defaultValue.asString();
+      value = value->default_value(defaultValue, defaultValueForHelp(defaultValue));
       if constexpr (V != VariantType::String) {
         value = value->multitoken();
       }
       options.add_options()(name, value, help);
+    } else if constexpr (isLabeledArray<V>()) {
+      // the JSON representation is passed as a single argument
+      auto value = boost::program_options::value<std::string>();
+      auto defaultValue = spec.defaultValue.asString();
+      value = value->default_value(defaultValue, defaultValueForHelp(defaultValue));
+      options.add_options()(name, value, help);
     } else {
       using Type = typename variant_type<V>::type;
       using BoostType = typename std::conditional<V == VariantType::String, std::string, Type>::type;

Framework/Core/include/Framework/Variant.h

@@ -13,6 +13,7 @@
 
 #include "Framework/RuntimeError.h"
 #include "Framework/Array2D.h"
+#include "Framework/Traits.h"
 #include <type_traits>
 #include <cstring>
 #include <cstdint>
@@ -261,9 +262,9 @@ struct variant_helper {
     new (reinterpret_cast<T*>(store)) T{};
     *(reinterpret_cast<T*>(store)) = value;
   }
-  static void set(void* store, T values, size_t size)
+  static void set(void* store, std::remove_pointer_t<T> const* values, size_t size)
   {
-    *reinterpret_cast<T*>(store) = reinterpret_cast<T>(std::memcpy(std::malloc(size * sizeof(std::remove_pointer_t<T>)), reinterpret_cast<void*>(values), size * sizeof(std::remove_pointer_t<T>)));
+    *reinterpret_cast<T*>(store) = reinterpret_cast<T>(std::memcpy(std::malloc(size * sizeof(std::remove_pointer_t<T>)), reinterpret_cast<void const*>(values), size * sizeof(std::remove_pointer_t<T>)));
   }
 
   static T get(const void* store) { return *(reinterpret_cast<const T*>(store)); }
@@ -317,9 +318,10 @@ struct variant_helper<std::string> {
 class Variant
 {
  public:
-  Variant(VariantType type = VariantType::Unknown) : mType{type} {}
+  Variant(VariantType type = VariantType::Unknown);
 
   template <typename T>
+    requires(!is_specialization_v<T, std::vector>)
   Variant(T value) : mType{variant_trait_v<T>}
   {
     variant_helper<decltype(value)>::set(&mStore, value);
@@ -331,17 +333,23 @@ class Variant
     variant_helper<T>::set(&mStore, values, mSize);
   }
 
+  // A Variant owns what it stores: the content of vectors is copied
   template <typename T>
-  Variant(std::vector<T>& values) : mType{variant_trait_v<T*>}, mSize{values.size()}
+  Variant(std::vector<T> const& values) : mType{variant_trait_v<T*>}, mSize{values.size()}
   {
     variant_helper<T*>::set(&mStore, values.data(), mSize);
   }
 
-  Variant(std::vector<std::string>& values) : mType{VariantType::ArrayString}, mSize{values.size()}
+  Variant(std::vector<std::string> const& values) : mType{VariantType::ArrayString}, mSize{values.size()}
   {
     variant_helper<std::vector<std::string>>::set(&mStore, values);
   }
 
+  // A temporary vector cannot hand over its buffer to a Variant,
+  // so creating one only to copy it is an error
+  template <typename T>
+  Variant(std::vector<T>&&) = delete;
+
   template <typename T>
   Variant(std::initializer_list<T>)
   {
@@ -355,6 +363,8 @@ class Variant
   ~Variant();
   Variant& operator=(const Variant& other);
   Variant& operator=(Variant&& other) noexcept;
+  // Assignment from a temporary vector has to be allowed because it is used, but copies
+  // to make sure Variant owns its content
   template <typename T>
   Variant& operator=(std::vector<T>&& other) noexcept
   {
@@ -381,31 +391,25 @@ class Variant
     return variant_helper<T>::get(&mStore);
   }
 
+  // The setters replace the current content, releasing it first.
   template <typename T>
   void set(T value)
   {
-    return variant_helper<T>::set(&mStore, value);
+    *this = Variant(value);
   }
 
   template <typename T>
   void set(T value, size_t size)
   {
-    mSize = size;
-    return variant_helper<T>::set(&mStore, value, mSize);
-  }
-
-  template <typename T>
-  void set(std::vector<T>& values)
-    requires(std::is_pod_v<T>)
-  {
-    return variant_helper<T*>::set(&mStore, values.data(), values.size());
+    *this = Variant(value, size);
   }
 
+  /// FIXME: set for vector of strings is not used anywhere, why?
   template <typename T>
   void set(std::vector<T>& values)
-    requires(std::is_same_v<T, std::string>)
+    requires(std::is_pod_v<T> || std::is_same_v<T, std::string>)
   {
-    return variant_helper<T*>::set(&mStore, values);
+    *this = Variant(values);
   }
 
   [[nodiscard]] VariantType type() const { return mType; }
@@ -414,6 +418,10 @@ class Variant
 
  private:
   friend std::ostream& operator<<(std::ostream& oss, Variant const& val);
+  // Helpers to manage the store depending on the actual content
+  void copyStore(Variant const& other);
+  void moveStore(Variant& other) noexcept;
+  void destroyStore() noexcept;
   using storage_t = std::aligned_union<8, int, int8_t, int16_t, int64_t,
                                        uint8_t, uint16_t, uint32_t, uint64_t,
                                        const char*, float, double, bool,

Framework/Core/include/Framework/VariantPropertyTreeHelpers.h

@@ -12,7 +12,6 @@
 #define FRAMEWORK_VARIANTPTREEHELPERS_H
 
 #include "Array2D.h"
-#include "Framework/Variant.h"
 #include <boost/property_tree/ptree.hpp>
 
 namespace o2::framework

Framework/Core/src/BoostOptionsRetriever.cxx

@@ -91,13 +91,13 @@ void BoostOptionsRetriever::update(std::vector<ConfigParamSpec> const& specs,
       case VariantType::Array2DInt:
       case VariantType::Array2DFloat:
       case VariantType::Array2DDouble:
-        options = options(name, bpo::value<std::string>()->default_value(spec.defaultValue.asString()), help);
-        break;
-      case VariantType::Dict:
       case VariantType::LabeledArrayInt:
       case VariantType::LabeledArrayFloat:
       case VariantType::LabeledArrayDouble:
       case VariantType::LabeledArrayString:
+        options = options(name, bpo::value<std::string>()->default_value(spec.defaultValue.asString()), help);
+        break;
+      case VariantType::Dict:
       case VariantType::Unknown:
       case VariantType::Empty:
         break;

Framework/Core/src/ConfigParamsHelper.cxx

@@ -13,6 +13,7 @@
 #include <boost/program_options.hpp>
 
 #include <string>
+#include <string_view>
 #include <vector>
 #include <iostream>
 
@@ -99,9 +100,17 @@ void ConfigParamsHelper::populateBoostProgramOptions(
         addConfigSpecOption<VariantType::Array2DDouble>(spec, options);
         break;
       case VariantType::LabeledArrayInt:
+        addConfigSpecOption<VariantType::LabeledArrayInt>(spec, options);
+        break;
       case VariantType::LabeledArrayFloat:
+        addConfigSpecOption<VariantType::LabeledArrayFloat>(spec, options);
+        break;
       case VariantType::LabeledArrayDouble:
+        addConfigSpecOption<VariantType::LabeledArrayDouble>(spec, options);
+        break;
       case VariantType::LabeledArrayString:
+        addConfigSpecOption<VariantType::LabeledArrayString>(spec, options);
+        break;
       // FIXME: for  Dict we should probably allow parsing stuff
       //        provided on the command line
       case VariantType::Dict:
@@ -112,6 +121,19 @@ void ConfigParamsHelper::populateBoostProgramOptions(
   }
 }
 
+std::string ConfigParamsHelper::defaultValueForHelp(std::string const& value)
+{
+  constexpr std::string_view ellipsis = "...";
+  if (value.size() <= MaxHelpDefaultLength) {
+    return value;
+  }
+  // Keep more of the beginning, which identifies the value, and the end, which closes it
+  constexpr size_t kept = MaxHelpDefaultLength - ellipsis.size();
+  constexpr size_t tail = kept / 3;
+  constexpr size_t head = kept - tail;
+  return value.substr(0, head) + std::string{ellipsis} + value.substr(value.size() - tail);
+}
+
 /// Check if option is defined
 bool ConfigParamsHelper::hasOption(const std::vector<ConfigParamSpec>& specs, const std::string& optName)
 {
@@ -158,7 +180,7 @@ bool ConfigParamsHelper::dpl2BoostOptions(const std::vector<ConfigParamSpec>& sp
     if (configSpec.type != VariantType::Bool) {
       if (configSpec.defaultValue.type() != VariantType::Empty) {
         options.add_options()(configSpec.name.c_str(),
-                              bpo::value<std::string>()->default_value(defaultValue.str()),
+                              bpo::value<std::string>()->default_value(defaultValue.str(), defaultValueForHelp(defaultValue.str())),
                               configSpec.help.c_str());
       } else {
         options.add_options()(configSpec.name.c_str(),

Framework/Core/src/PropertyTreeHelpers.cxx

@@ -35,6 +35,32 @@ inline Variant fromString(std::string const& str)
   ss.str(str);
   return VariantJSONHelpers::read<T>(ss);
 }
+
+template <typename T>
+auto replaceLabels(LabeledArray<T>& input, LabeledArray<T>&& spec)
+{
+  if (!input.getLabelsCols().empty() || !input.getLabelsRows().empty()) {
+    return false;
+  }
+  if (spec.getLabelsCols().empty() == false) {
+    input.replaceLabelsCols(spec.getLabelsCols());
+  }
+  if (spec.getLabelsRows().empty() == false) {
+    input.replaceLabelsRows(spec.getLabelsRows());
+  }
+  return true;
+}
+
+/// Special case for a labeled array to restore default labels that were omitted
+template <typename T, VariantType V>
+boost::property_tree::ptree labeledArrayFromStringToBranch(std::string const& str, Variant const& defaultValue)
+{
+  auto v = fromString<V>(str).template get<LabeledArray<T>>();
+  if (defaultValue.type() == V) {
+    replaceLabels(v, defaultValue.get<LabeledArray<T>>());
+  }
+  return labeledArrayToBranch(std::move(v));
+}
 } // namespace
 void PropertyTreeHelpers::populateDefaults(std::vector<ConfigParamSpec> const& schema,
                                            boost::property_tree::ptree& pt,
@@ -233,22 +259,18 @@ void PropertyTreeHelpers::populate(std::vector<ConfigParamSpec> const& schema,
           auto v = fromString<VariantType::Array2DDouble>(vmap[key].as<std::string>());
           pt.put_child(key, array2DToBranch<double>(v.get<Array2D<double>>()));
         } break;
-        case VariantType::LabeledArrayInt: {
-          auto v = fromString<VariantType::LabeledArrayInt>(vmap[key].as<std::string>());
-          pt.put_child(key, labeledArrayToBranch(v.get<LabeledArray<int>>()));
-        } break;
-        case VariantType::LabeledArrayFloat: {
-          auto v = fromString<VariantType::LabeledArrayFloat>(vmap[key].as<std::string>());
-          pt.put_child(key, labeledArrayToBranch(v.get<LabeledArray<float>>()));
-        } break;
-        case VariantType::LabeledArrayDouble: {
-          auto v = fromString<VariantType::LabeledArrayDouble>(vmap[key].as<std::string>());
-          pt.put_child(key, labeledArrayToBranch(v.get<LabeledArray<double>>()));
-        } break;
-        case VariantType::LabeledArrayString: {
-          auto v = fromString<VariantType::LabeledArrayString>(vmap[key].as<std::string>());
-          pt.put_child(key, labeledArrayToBranch(v.get<LabeledArray<std::string>>()));
-        } break;
+        case VariantType::LabeledArrayInt:
+          pt.put_child(key, labeledArrayFromStringToBranch<int, VariantType::LabeledArrayInt>(vmap[key].as<std::string>(), spec.defaultValue));
+          break;
+        case VariantType::LabeledArrayFloat:
+          pt.put_child(key, labeledArrayFromStringToBranch<float, VariantType::LabeledArrayFloat>(vmap[key].as<std::string>(), spec.defaultValue));
+          break;
+        case VariantType::LabeledArrayDouble:
+          pt.put_child(key, labeledArrayFromStringToBranch<double, VariantType::LabeledArrayDouble>(vmap[key].as<std::string>(), spec.defaultValue));
+          break;
+        case VariantType::LabeledArrayString:
+          pt.put_child(key, labeledArrayFromStringToBranch<std::string, VariantType::LabeledArrayString>(vmap[key].as<std::string>(), spec.defaultValue));
+          break;
         case VariantType::Dict:
           pt.put_child(key, vmap[key].as<boost::property_tree::ptree>());
           break;
@@ -272,21 +294,6 @@ void PropertyTreeHelpers::populate(std::vector<ConfigParamSpec> const& schema,
   O2_SIGNPOST_END(configuration, cid, "populate", "Done");
 }
 
-template <typename T>
-auto replaceLabels(LabeledArray<T>& input, LabeledArray<T>&& spec)
-{
-  if (!input.getLabelsCols().empty() || !input.getLabelsRows().empty()) {
-    return false;
-  }
-  if (spec.getLabelsCols().empty() == false) {
-    input.replaceLabelsCols(spec.getLabelsCols());
-  }
-  if (spec.getLabelsRows().empty() == false) {
-    input.replaceLabelsRows(spec.getLabelsRows());
-  }
-  return true;
-}
-
 void PropertyTreeHelpers::populate(std::vector<ConfigParamSpec> const& schema,
                                    boost::property_tree::ptree& pt,
                                    boost::property_tree::ptree const& in,