[RF] New RooStringView class for passing std::strings to functions #9747
Conversation
Many RooFit function take a C-style string `const char*` as an argument.
This commit proposes a mechanism to generalize such interfaces to also
accepts `std::string`.
Two options would have been the following, but they both have drawbacks:
1. Use `std::string const&` directly. Problem: the C-style string is
copied implicitly to create the `std::string`.
3. Use `std::string_view`. Problem: a `std::string_view` is not
necessarily null-terminated, to converting it to a C-style string
involves again a copy.
The alternative copy-fee solution that this commit suggests is a C-style
string wrapper called `RooStringView`, that can be implicitely
constructed from a `std::string` as well. Like this, there will be no
memory reallocation/copying when one passes a `const char*`.
The New RooStringView is used in this commit for the first time to
generalize the RooWorkspace accessor functions to also accept
`std::string`.
Some code modernization, where we profit from the new possibility to pass `std::string` directly to the RooWorkspace accessor functions thanks to the `RooStringView`.
This has two advantages over `std::string_view`:
1. We don't need preporcessor macros and branches to support ROOT's
std::string_view backport for C++14.
2. The `std::string_view` was not appropriate anyway, because in the
`TNamed` constructor the strig had to be copied anyway to guarantee
the string is null-terminated. With `RooStringView` we don't have
this problem: it is guaranteed to be null-terminated.
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.
|
Starting build on |
roofit/roofitcore/inc/RooWorkspace.h
Outdated
| inline RooArgSet argSet(std::string const& nameList) const { return argSet(nameList.c_str()); } | ||
| inline TObject* genobj(std::string const& name) const { return genobj(name.c_str()); } | ||
| inline TObject* obj(std::string const& name) const { return obj(name.c_str()); } | ||
|
|
There was a problem hiding this comment.
Why do we need these extra functions ?
There was a problem hiding this comment.
I am referring here to componentIterator and components
There was a problem hiding this comment.
I don't know where they are used exactly, but they were there before so I would suggest to not remove them from the interface
|
Build failed on mac11/cxx17. Warnings:
And 1 more Failing tests:
|
guitargeek
force-pushed
the
rooworkspace_1
branch
from
b6eb530 to
8ae5938
Compare
|
Starting build on |
guitargeek
changed the title
std::string overloads for RooWorkspace accessor functions
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests: |
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
|
Build failed on ROOT-ubuntu2004/soversion. Failing tests: |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
|
Build failed on mac11/cxx17. Failing tests: |
|
Build failed on mac1015/python3. Failing tests: |
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests: |
|
Hi @lmoneta, thanks for the review! You are right, it's not a sustainable solution to duplicate the interfaces if we want to accept |
|
@guitargeek , I am trying to test the latest root master changes and notice that cmssw code https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/Utilities/src/SideBandSubtraction.cc#L415 fails due to this change. Should I update cmssw to use something like the following now? |
A new
RooStringViewis introduced as a copy-free drop-in replacement forconst char*in public RooFit interfaces, which also accepts astd::string.So far, this new
RooStringViewis used in the RooWorkspace accessors, and also in the dataset classes as it's a superior solution over usingstd::stringview, which is not guaranteed to be null-terminated and needs specific preprocessor macros for the C++ case.