[RF][HS3] Reduce code duplication in RooFit HS3

Refactor the HS3 serialization code to remove duplicated logic without
any change in behavior. All hs3, histfactory and JSON tests still pass.

JSONFactories_RooFitCore.cxx:
- Drop RooLegacyExpPolyFactory; it was identical to the existing
  RooPolynomialFactory<RooLegacyExpPoly>, which is now registered directly.
- Drop RooLegacyExpPolyStreamer; it was identical to
  RooPolynomialStreamer<RooLegacyExpPoly>.
- Merge RooHistFuncStreamer/RooHistPdfStreamer into RooHistStreamer<T> and
  RooHistFuncFactory/RooHistPdfFactory into RooHistFactory<T>; each pair
  differed only by the concrete RooFit type.

RooJSONFactoryWSTool.cxx:
- Remove fillSeqSanitizedName, which had no callers anywhere in ROOT.
- Factor the "make a valid name but refuse to change the first character"
  block (exportCategory, exportCombinedData) into makeValidNameOrError().
- Collapse the repeated ofstream/ifstream open-and-error boilerplate in
  exportJSON/exportYML/importJSON/importYML. error() is [[noreturn]], so
  the trailing 'return false' statements were dead code.

JSONFactories_HistFactory.cxx:
- Remove a redundant mid-file block of #includes (<regex> was already
  included; the others were already used earlier in the translation unit).

JSONIO.cxx:
- Deduplicate removeImporters/removeExporters and printImporters/
  printExporters into shared templated helpers.

Author: guitargeek

roofit/hs3/inc/RooFitHS3/RooJSONFactoryWSTool.h

@@ -63,7 +63,6 @@ class RooJSONFactoryWSTool {
    static RooFit::Detail::JSONNode const *findNamedChild(RooFit::Detail::JSONNode const &node, std::string const &name);
 
    static void fillSeq(RooFit::Detail::JSONNode &node, RooAbsCollection const &coll, size_t nMax = -1);
-   static void fillSeqSanitizedName(RooFit::Detail::JSONNode &node, RooAbsCollection const &coll, size_t nMax = -1);
 
    template <class T>
    T *request(const std::string &objname, const std::string &requestAuthor)

roofit/hs3/src/JSONFactories_HistFactory.cxx

@@ -765,12 +765,6 @@ std::vector<std::string> splitTopLevelProduct(const std::string &expr)
    return parts;
 }
 
-#include <regex>
-#include <string>
-#include <cctype>
-#include <cstdlib>
-#include <iostream>
-
 NormSys parseOverallModifierFormula(const std::string &s, RooFormulaVar *formula)
 {
    static const std::regex pattern(

roofit/hs3/src/JSONFactories_RooFitCore.cxx

@@ -499,37 +499,6 @@ class RooExponentialFactory : public RooFit::JSONIO::Importer {
    }
 };
 
-class RooLegacyExpPolyFactory : public RooFit::JSONIO::Importer {
-public:
-   bool importArg(RooJSONFactoryWSTool *tool, const JSONNode &p) const override
-   {
-      std::string name(RooJSONFactoryWSTool::name(p));
-      if (!p.has_child("coefficients")) {
-         RooJSONFactoryWSTool::error("no coefficients given in '" + name + "'");
-      }
-      RooAbsReal *x = tool->requestArg<RooAbsReal>(p, "x");
-      RooArgList coefs;
-      int order = 0;
-      int lowestOrder = 0;
-      for (const auto &coef : p["coefficients"].children()) {
-         // As long as the coefficients match the default coefficients in
-         // RooFit, we don't have to instantiate RooFit objects but can
-         // increase the lowestOrder flag.
-         if (order == 0 && coef.val() == "1.0") {
-            ++lowestOrder;
-         } else if (coefs.empty() && coef.val() == "0.0") {
-            ++lowestOrder;
-         } else {
-            coefs.add(*tool->request<RooAbsReal>(coef.val(), name));
-         }
-         ++order;
-      }
-
-      tool->wsEmplace<RooLegacyExpPoly>(name, *x, coefs, lowestOrder);
-      return true;
-   }
-};
-
 class RooMultiVarGaussianFactory : public RooFit::JSONIO::Importer {
 public:
    bool importArg(RooJSONFactoryWSTool *tool, const JSONNode &p) const override
@@ -743,48 +712,22 @@ class RooRealSumFuncStreamer : public RooFit::JSONIO::Exporter {
    }
 };
 
-class RooHistFuncStreamer : public RooFit::JSONIO::Exporter {
-public:
-   std::string const &key() const override;
-   bool exportObject(RooJSONFactoryWSTool *tool, const RooAbsArg *func, JSONNode &elem) const override
-   {
-      const RooHistFunc *hf = static_cast<const RooHistFunc *>(func);
-      elem["type"] << key();
-      RooDataHist const &dh = hf->dataHist();
-      tool->exportHisto(*dh.get(), dh.numEntries(), dh.weightArray(), elem["data"].set_map());
-      return true;
-   }
-};
-
-class RooHistFuncFactory : public RooFit::JSONIO::Importer {
-public:
-   bool importArg(RooJSONFactoryWSTool *tool, const JSONNode &p) const override
-   {
-      std::string name(RooJSONFactoryWSTool::name(p));
-      if (!p.has_child("data")) {
-         RooJSONFactoryWSTool::error("function '" + name + "' is of histogram type, but does not define a 'data' key");
-      }
-      std::unique_ptr<RooDataHist> dataHist =
-         RooJSONFactoryWSTool::readBinnedData(p["data"], name, RooJSONFactoryWSTool::readAxes(p["data"]));
-      tool->wsEmplace<RooHistFunc>(name, *dataHist->get(), *dataHist);
-      return true;
-   }
-};
-
-class RooHistPdfStreamer : public RooFit::JSONIO::Exporter {
+template <class RooArg_t>
+class RooHistStreamer : public RooFit::JSONIO::Exporter {
 public:
    std::string const &key() const override;
    bool exportObject(RooJSONFactoryWSTool *tool, const RooAbsArg *func, JSONNode &elem) const override
    {
-      const RooHistPdf *hf = static_cast<const RooHistPdf *>(func);
+      const RooArg_t *hf = static_cast<const RooArg_t *>(func);
       elem["type"] << key();
       RooDataHist const &dh = hf->dataHist();
       tool->exportHisto(*dh.get(), dh.numEntries(), dh.weightArray(), elem["data"].set_map());
       return true;
    }
 };
 
-class RooHistPdfFactory : public RooFit::JSONIO::Importer {
+template <class RooArg_t>
+class RooHistFactory : public RooFit::JSONIO::Importer {
 public:
    bool importArg(RooJSONFactoryWSTool *tool, const JSONNode &p) const override
    {
@@ -794,7 +737,7 @@ class RooHistPdfFactory : public RooFit::JSONIO::Importer {
       }
       std::unique_ptr<RooDataHist> dataHist =
          RooJSONFactoryWSTool::readBinnedData(p["data"], name, RooJSONFactoryWSTool::readAxes(p["data"]));
-      tool->wsEmplace<RooHistPdf>(name, *dataHist->get(), *dataHist);
+      tool->wsEmplace<RooArg_t>(name, *dataHist->get(), *dataHist);
       return true;
    }
 };
@@ -893,17 +836,6 @@ class RooPolynomialStreamer : public RooFit::JSONIO::Exporter {
    }
 };
 
-class RooLegacyExpPolyStreamer : public RooFit::JSONIO::Exporter {
-public:
-   std::string const &key() const override;
-   bool exportObject(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem) const override
-   {
-      elem["type"] << key();
-      writePolynomialBody(static_cast<const RooLegacyExpPoly *>(func), elem);
-      return true;
-   }
-};
-
 class RooPoissonStreamer : public RooFit::JSONIO::Exporter {
 public:
    std::string const &key() const override;
@@ -1248,14 +1180,17 @@ DEFINE_EXPORTER_KEY(RooAddPdfStreamer<RooAddModel>, "mixture_model");
 DEFINE_EXPORTER_KEY(RooBinSamplingPdfStreamer, "binsampling");
 DEFINE_EXPORTER_KEY(RooWrapperPdfStreamer, "density_function_dist");
 DEFINE_EXPORTER_KEY(RooBinWidthFunctionStreamer, "binwidth");
-DEFINE_EXPORTER_KEY(RooLegacyExpPolyStreamer, "legacy_exp_poly_dist");
+template <>
+DEFINE_EXPORTER_KEY(RooPolynomialStreamer<RooLegacyExpPoly>, "legacy_exp_poly_dist");
 DEFINE_EXPORTER_KEY(RooExponentialStreamer, "exponential_dist");
 template <>
 DEFINE_EXPORTER_KEY(RooFormulaArgStreamer<RooFormulaVar>, "generic_function");
 template <>
 DEFINE_EXPORTER_KEY(RooFormulaArgStreamer<RooGenericPdf>, "generic_dist");
-DEFINE_EXPORTER_KEY(RooHistFuncStreamer, "histogram");
-DEFINE_EXPORTER_KEY(RooHistPdfStreamer, "histogram_dist");
+template <>
+DEFINE_EXPORTER_KEY(RooHistStreamer<RooHistFunc>, "histogram");
+template <>
+DEFINE_EXPORTER_KEY(RooHistStreamer<RooHistPdf>, "histogram_dist");
 DEFINE_EXPORTER_KEY(RooLogNormalStreamer, "lognormal_dist");
 DEFINE_EXPORTER_KEY(RooMultiVarGaussianStreamer, "multivariate_normal_dist");
 DEFINE_EXPORTER_KEY(RooPoissonStreamer, "poisson_dist");
@@ -1292,12 +1227,12 @@ STATIC_EXECUTE([]() {
    registerImporter<RooAddModelFactory>("mixture_model", false);
    registerImporter<RooBinSamplingPdfFactory>("binsampling_dist", false);
    registerImporter<RooBinWidthFunctionFactory>("binwidth", false);
-   registerImporter<RooLegacyExpPolyFactory>("legacy_exp_poly_dist", false);
+   registerImporter<RooPolynomialFactory<RooLegacyExpPoly>>("legacy_exp_poly_dist", false);
    registerImporter<RooExponentialFactory>("exponential_dist", false);
    registerImporter<RooFormulaArgFactory<RooFormulaVar>>("generic_function", false);
    registerImporter<RooFormulaArgFactory<RooGenericPdf>>("generic_dist", false);
-   registerImporter<RooHistFuncFactory>("histogram", false);
-   registerImporter<RooHistPdfFactory>("histogram_dist", false);
+   registerImporter<RooHistFactory<RooHistFunc>>("histogram", false);
+   registerImporter<RooHistFactory<RooHistPdf>>("histogram_dist", false);
    registerImporter<RooLogNormalFactory>("lognormal_dist", false);
    registerImporter<RooMultiVarGaussianFactory>("multivariate_normal_dist", false);
    registerImporter<RooPoissonFactory>("poisson_dist", false);
@@ -1320,12 +1255,12 @@ STATIC_EXECUTE([]() {
    registerExporter<RooAddPdfStreamer<RooAddModel>>(RooAddModel::Class(), false);
    registerExporter<RooBinSamplingPdfStreamer>(RooBinSamplingPdf::Class(), false);
    registerExporter<RooBinWidthFunctionStreamer>(RooBinWidthFunction::Class(), false);
-   registerExporter<RooLegacyExpPolyStreamer>(RooLegacyExpPoly::Class(), false);
+   registerExporter<RooPolynomialStreamer<RooLegacyExpPoly>>(RooLegacyExpPoly::Class(), false);
    registerExporter<RooExponentialStreamer>(RooExponential::Class(), false);
    registerExporter<RooFormulaArgStreamer<RooFormulaVar>>(RooFormulaVar::Class(), false);
    registerExporter<RooFormulaArgStreamer<RooGenericPdf>>(RooGenericPdf::Class(), false);
-   registerExporter<RooHistFuncStreamer>(RooHistFunc::Class(), false);
-   registerExporter<RooHistPdfStreamer>(RooHistPdf::Class(), false);
+   registerExporter<RooHistStreamer<RooHistFunc>>(RooHistFunc::Class(), false);
+   registerExporter<RooHistStreamer<RooHistPdf>>(RooHistPdf::Class(), false);
    registerExporter<RooLogNormalStreamer>(RooLognormal::Class(), false);
    registerExporter<RooMultiVarGaussianStreamer>(RooMultiVarGaussian::Class(), false);
    registerExporter<RooPoissonStreamer>(RooPoisson::Class(), false);

roofit/hs3/src/JSONIO.cxx

@@ -147,26 +147,13 @@ bool registerExporter(const std::string &key, std::unique_ptr<const Exporter> f,
    return true;
 }
 
-int removeImporters(const std::string &needle)
-{
-   int n = 0;
-   for (auto &element : importers()) {
-      for (size_t i = element.second.size(); i > 0; --i) {
-         auto *imp = element.second[i - 1].get();
-         std::string name(typeid(*imp).name());
-         if (name.find(needle) != std::string::npos) {
-            element.second.erase(element.second.begin() + i - 1);
-            ++n;
-         }
-      }
-   }
-   return n;
-}
+namespace {
 
-int removeExporters(const std::string &needle)
+template <class Map>
+int removeByTypeName(Map &map, const std::string &needle)
 {
    int n = 0;
-   for (auto &element : exporters()) {
+   for (auto &element : map) {
       for (size_t i = element.second.size(); i > 0; --i) {
          auto *imp = element.second[i - 1].get();
          std::string name(typeid(*imp).name());
@@ -179,25 +166,37 @@ int removeExporters(const std::string &needle)
    return n;
 }
 
-void printImporters()
+template <class Map, class KeyPrinter>
+void printByTypeName(Map &map, KeyPrinter printKey)
 {
-   for (const auto &x : importers()) {
+   for (const auto &x : map) {
       for (const auto &ePtr : x.second) {
          // Passing *e directory to typeid results in clang warnings.
          auto const &e = *ePtr;
-         std::cout << x.first << "\t" << typeid(e).name() << std::endl;
+         std::cout << printKey(x.first) << "\t" << typeid(e).name() << std::endl;
       }
    }
 }
+
+} // namespace
+
+int removeImporters(const std::string &needle)
+{
+   return removeByTypeName(importers(), needle);
+}
+
+int removeExporters(const std::string &needle)
+{
+   return removeByTypeName(exporters(), needle);
+}
+
+void printImporters()
+{
+   printByTypeName(importers(), [](auto const &key) { return key; });
+}
 void printExporters()
 {
-   for (const auto &x : exporters()) {
-      for (const auto &ePtr : x.second) {
-         // Passing *e directory to typeid results in clang warnings.
-         auto const &e = *ePtr;
-         std::cout << x.first->GetName() << "\t" << typeid(e).name() << std::endl;
-      }
-   }
+   printByTypeName(exporters(), [](auto const &key) { return key->GetName(); });
 }
 
 void loadFactoryExpressions(const std::string &fname)