From 77488f14eebb8b6e2e5dc72736a616fa20d1b269 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 31 Aug 2026 13:49:35 +0000 Subject: [PATCH] [RF] Make the code emitted by codegen locale-independent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The codegen backend formats the numbers it writes into the generated C++ with default-constructed streams, which follow the global locale. Under a comma-decimal locale (a German one, say) the emitted literals came out as "0,5": that does not compile, and inside a call argument list the comma even turns one argument into two, so the generated code silently changes meaning instead of failing. Imbue the classic locale on every stream whose output ends up in the generated code: CodegenImpl's doubleToString(), which formats the value of RooConstVar/RooRealVar and the analytical integrals, plus CodegenContext's buildArg() for double arguments, buildArgSpanImpl() for the emitted arrays (where a grouping separator would corrupt integers the same way), and the stream that renders the node descriptions into the code comments. Add a regression test that generates the code for a RooGaussian with constant parameters and for a RooChebychev (whose observable range is passed to the generated call as a plain double) under a comma-decimal locale, built from a custom numpunct facet because no such OS locale is installed everywhere. Both must produce exactly the code they produce under the classic locale. This is the first test for the codegen package, so it also adds roofit/codegen/test/. 🤖 Done with the help of AI --- roofit/codegen/CMakeLists.txt | 2 + roofit/codegen/src/CodegenImpl.cxx | 4 + roofit/codegen/test/CMakeLists.txt | 1 + roofit/codegen/test/testCodegen.cxx | 114 ++++++++++++++++++ roofit/roofitcore/inc/RooFit/CodegenContext.h | 3 + .../roofitcore/src/RooFit/CodegenContext.cxx | 2 + 6 files changed, 126 insertions(+) create mode 100644 roofit/codegen/test/CMakeLists.txt create mode 100644 roofit/codegen/test/testCodegen.cxx diff --git a/roofit/codegen/CMakeLists.txt b/roofit/codegen/CMakeLists.txt index 664dace90c5ce..e1fecef63dac2 100644 --- a/roofit/codegen/CMakeLists.txt +++ b/roofit/codegen/CMakeLists.txt @@ -34,3 +34,5 @@ if(NOT CMAKE_VERSION VERSION_LESS "3.23.0") inc/RooFit/CodegenImpl.h ) endif() + +ROOT_ADD_TEST_SUBDIRECTORY(test) diff --git a/roofit/codegen/src/CodegenImpl.cxx b/roofit/codegen/src/CodegenImpl.cxx index bc3bdc3556d3e..750f6912bc380 100644 --- a/roofit/codegen/src/CodegenImpl.cxx +++ b/roofit/codegen/src/CodegenImpl.cxx @@ -66,6 +66,7 @@ #include +#include #include namespace RooFit::Experimental { @@ -76,6 +77,9 @@ namespace { std::string doubleToString(double val) { std::stringstream ss; + // The formatting must not depend on the global locale: a comma decimal + // separator (e.g. from a German locale) would corrupt the generated C++. + ss.imbue(std::locale::classic()); ss << std::setprecision(std::numeric_limits::max_digits10) << val; return ss.str(); } diff --git a/roofit/codegen/test/CMakeLists.txt b/roofit/codegen/test/CMakeLists.txt new file mode 100644 index 0000000000000..6ec0ce7f477b1 --- /dev/null +++ b/roofit/codegen/test/CMakeLists.txt @@ -0,0 +1 @@ +ROOT_ADD_GTEST(testCodegen testCodegen.cxx LIBRARIES RooFitCore RooFit RooFitCodegen) diff --git a/roofit/codegen/test/testCodegen.cxx b/roofit/codegen/test/testCodegen.cxx new file mode 100644 index 0000000000000..e6c38407d7081 --- /dev/null +++ b/roofit/codegen/test/testCodegen.cxx @@ -0,0 +1,114 @@ +// Tests for the C++ code that the RooFit codegen backend generates. +// Author: Jonas Rembser, CERN 2026 + +#include + +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +namespace { + +/// Formats decimal points as ',' like a German locale does. Such a locale is +/// not installed on every test machine, so it is built from a custom facet +/// instead of requested by name. The resulting locale is unnamed, so making it +/// global does not also switch the C locale that std::strtod() uses. +struct CommaPunct : std::numpunct { + char do_decimal_point() const override { return ','; } +}; + +/// Generate the code for `arg` with `loc` as the global locale, restoring the +/// previous global locale even if code generation throws. +std::string codeUnderLocale(RooAbsArg &arg, std::locale const &loc) +{ + const std::locale old = std::locale::global(loc); + std::string code; + try { + RooFit::Experimental::CodegenContext ctx; + ctx.buildFunction(arg); + code = ctx.collectedCode(); + } catch (...) { + std::locale::global(old); + throw; + } + std::locale::global(old); + return code; +} + +/// Erase the global counter from the generated function name, which differs +/// between two code generations of the same model. +std::string normalized(std::string const &code) +{ + return std::regex_replace(code, std::regex{"roo_codegen_[0-9]+"}, "roo_codegen_N"); +} + +/// How a plain stream formats 0.5 under `loc`, to verify that the facet is +/// actually in effect (otherwise the tests below would pass vacuously). +std::string formatWithStream(double val, std::locale const &loc) +{ + const std::locale old = std::locale::global(loc); + std::stringstream ss; + ss << val; + std::locale::global(old); + return ss.str(); +} + +} // namespace + +// The generated code is C++ source, so its number formatting must not follow +// the global locale: under a comma-decimal locale the literals came out as +// "0,5", which does not compile, and inside a function call argument list the +// comma even turns one argument into two. +TEST(RooFitCodegen, ValueLiteralsAreLocaleIndependent) +{ + const std::locale comma{std::locale::classic(), new CommaPunct}; + ASSERT_EQ(formatWithStream(0.5, comma), "0,5"); + + RooRealVar x{"x", "x", 0.5, -10, 10}; + RooRealVar mean{"mean", "mean", 1.25}; + RooConstVar sigma{"sigma", "sigma", 0.75}; + RooGaussian gauss{"gauss", "gauss", x, mean, sigma}; + x.setConstant(true); + mean.setConstant(true); + + const std::string code = codeUnderLocale(gauss, comma); + + EXPECT_EQ(normalized(code), normalized(codeUnderLocale(gauss, std::locale::classic()))); + for (std::string const &literal : {"0.5", "1.25", "0.75"}) { + EXPECT_NE(code.find(literal), std::string::npos) << literal << " missing from:\n" << code; + } + for (std::string const &corrupted : {"0,5", "1,25", "0,75"}) { + EXPECT_EQ(code.find(corrupted), std::string::npos) << corrupted << " emitted in:\n" << code; + } +} + +// Same for the doubles that the codegen implementations pass to the generated +// function calls directly (here the observable range of RooChebychev), which +// are formatted by CodegenContext::buildArg() and not by codegen's +// doubleToString(). +TEST(RooFitCodegen, CallArgumentsAreLocaleIndependent) +{ + const std::locale comma{std::locale::classic(), new CommaPunct}; + + RooRealVar x{"x", "x", 0.125, -0.5, 2.25}; + RooRealVar a1{"a1", "a1", 0.375}; + RooChebychev cheby{"cheby", "cheby", x, a1}; + + const std::string code = codeUnderLocale(cheby, comma); + + EXPECT_EQ(normalized(code), normalized(codeUnderLocale(cheby, std::locale::classic()))); + for (std::string const &literal : {"-0.5", "2.25", "0.375"}) { + EXPECT_NE(code.find(literal), std::string::npos) << literal << " missing from:\n" << code; + } + for (std::string const &corrupted : {"0,5", "2,25", "0,375"}) { + EXPECT_EQ(code.find(corrupted), std::string::npos) << corrupted << " emitted in:\n" << code; + } +} diff --git a/roofit/roofitcore/inc/RooFit/CodegenContext.h b/roofit/roofitcore/inc/RooFit/CodegenContext.h index f61b3f08450bb..18e70482c5453 100644 --- a/roofit/roofitcore/inc/RooFit/CodegenContext.h +++ b/roofit/roofitcore/inc/RooFit/CodegenContext.h @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -139,6 +140,7 @@ class CodegenContext { std::string buildArg(T x) { std::stringstream ss; + ss.imbue(std::locale::classic()); // the generated code is C++, not locale-dependent text ss << std::setprecision(std::numeric_limits::max_digits10) << x; return ss.str(); } @@ -215,6 +217,7 @@ std::string CodegenContext::buildArgSpanImpl(std::span arr) unsigned int n = arr.size(); std::string arrName = getTmpVarName(); std::stringstream ss; + ss.imbue(std::locale::classic()); // the generated code is C++, not locale-dependent text ss << typeName() << " " << arrName << "[" << n << "] = {"; for (unsigned int i = 0; i < n; i++) { ss << " " << arr[i] << ","; diff --git a/roofit/roofitcore/src/RooFit/CodegenContext.cxx b/roofit/roofitcore/src/RooFit/CodegenContext.cxx index 8a1f10e7c958e..72338604602da 100644 --- a/roofit/roofitcore/src/RooFit/CodegenContext.cxx +++ b/roofit/roofitcore/src/RooFit/CodegenContext.cxx @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -285,6 +286,7 @@ std::string CodegenContext::buildArg(std::span arr) CodegenContext::ScopeRAII::ScopeRAII(RooAbsArg const *arg, CodegenContext &ctx) : _ctx(ctx), _arg(arg) { std::ostringstream os; + os.imbue(std::locale::classic()); // the generated code is C++, not locale-dependent text Option_t *opts = nullptr; arg->printStream(os, _arg->defaultPrintContents(opts), _arg->defaultPrintStyle(opts)); _fn = os.str();