Skip to content

Commit b553d38

Browse files
committed
[RF] Fix ambiguity when calling RooDataHist constructor in PyROOT
The RooDataHist has two similar constructors: ```C++ RooDataHist(RooStringView name, RooStringView title, const RooArgSet& vars, const char* binningName); RooDataHist(RooStringView name, RooStringView title, const RooArgList& vars, const RooCmdArg& arg1); ``` The problem: a `RooCmdArg` can be implicitly constructed from a `const char*`. Therefore, if one doesn't explicitly choose one of the two overloads by passing a RooArgSet or RooArgList as the third argument explicitly, both overloads are succeeding in PyROOT. Unlike in C++, the overload that doesn't require implicit conversion is not prioritized. Here is an example of where this goes wrong: ```Python ROOT.RooDataHist(name, title, variables, "my_binning") ``` ...causing... ``` [#0] ERROR:InputArguments -- RooDataHist::ctor() ERROR: unrecognized command: my_binning ``` ...because the constructor with the RooCmdArg is used. It was sheer luck that this problem didn't appear so far. It only appeared after the the constructors were changed to use `RooStringView`, probably because the order is which the overloads are tried is not well defined. The solution: make the implicit construction of `RooCmdArg` from just a string impossible. This is done by requiring at least one non-default payload parameter. This is not breaking any code, because constructing a RooCmdArg without any payload doesn't make sense anyway.
1 parent 51c8b8f commit b553d38

1 file changed

Lines changed: 7 additions & 1 deletion

File tree

roofit/roofitcore/inc/RooCmdArg.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,14 @@ class RooCmdArg final : public TNamed {
2727
public:
2828

2929
RooCmdArg();
30+
/// Constructor from payload parameters. Note that the first payload
31+
/// parameter has no default value, because otherwise the implicit creation
32+
/// of a RooCmdArg from `const char*` would be possible. This would cause
33+
/// ambiguity problems in RooFit code. It is not a problem that the first
34+
/// parameter always has to be given, because creating a RooCmdArg with only
35+
/// a name and no payload doesn't make sense anyway.
3036
RooCmdArg(const char* name,
31-
Int_t i1=0, Int_t i2=0,
37+
Int_t i1, Int_t i2=0,
3238
Double_t d1=0, Double_t d2=0,
3339
const char* s1=0, const char* s2=0,
3440
const TObject* o1=0, const TObject* o2=0, const RooCmdArg* ca=0, const char* s3=0,

0 commit comments

Comments
 (0)