[math] Use std::function in ROOT::Math::ParamFunctor - #23211
Conversation
2867e5e to
3b30a35
Compare
| typedef T (* FreeFunc ) (T * , double *); | ||
| ParamFunctorTempl(FreeFunc f) : | ||
| fImpl(new ParamFunctorHandler<ParamFunctorTempl<T>,FreeFunc>(f) ) | ||
| ParamFunctorTempl(const PtrObj &p, MemFn memFn) |
There was a problem hiding this comment.
Why is this declared as a reference if it's supposed to be instantiated with pointers? Have you considered:
template <class Obj, typename MemFn>
ParamFunctorTempl(const Obj *p, MemFn memFn)
If you allow references here, I could try to instantiate it with a real object, and then it would either break at the capture or when trying to dereference the object for the call, wouldn't it?
There was a problem hiding this comment.
You're right that the reference buys nothing here. Changed to take a pointer:
template <class Obj, typename MemFn>
ParamFunctorTempl(Obj *p, MemFn memFn)I went with Obj * rather than const Obj *, because const Obj * would make (*p).*memFn a call on a const Obj & and reject non-const member functions, which TF1 is allowing. And since Obj is a template parameter, it can also be deduced to a const type if appropriate.
| // specialization used in TF1 | ||
| ParamFunctorTempl(std::function<Signature> f) : fFunc{std::move(f)} {} | ||
|
|
||
| T operator()(T *x, double *p) const { return fFunc(x, p); } |
There was a problem hiding this comment.
Isn't this one redundant? The const version below can be called also with pointers to non-const objects.
| T operator()(T *x, double *p) const { return fFunc(x, p); } |
There was a problem hiding this comment.
You're right, it was redundant. I removed it.
| return (*fImpl)(x,p); | ||
| } | ||
| // specialization used in TF1 | ||
| typedef T (*FreeFunc)(T *, double *); |
There was a problem hiding this comment.
Consider using FreeFunc = as was used above.
There was a problem hiding this comment.
This alias is gone now anyway, as a side effect of how your other comment #23211 (comment) was addressed.
| /// Construct from any callable object, or from a pointer to one. | ||
| template <typename Func, typename = std::enable_if_t<!std::is_same_v<std::decay_t<Func>, ParamFunctorTempl<T>>>> | ||
| explicit ParamFunctorTempl(Func f) : fFunc{Adapt(std::move(f))} | ||
| { | ||
| } | ||
|
|
||
| /** | ||
| Destructor (no operations) | ||
| */ | ||
| virtual ~ParamFunctorTempl () { | ||
| if (fImpl) delete fImpl; | ||
| } | ||
|
|
||
| /** | ||
| Copy constructor | ||
| */ | ||
| ParamFunctorTempl(const ParamFunctorTempl & rhs) : | ||
| fImpl(nullptr) | ||
| { | ||
| // if (rhs.fImpl.get() != 0) | ||
| // fImpl = std::unique_ptr<Impl>( (rhs.fImpl)->Clone() ); | ||
| if (rhs.fImpl) fImpl = rhs.fImpl->Clone(); | ||
| } | ||
|
|
||
| /** | ||
| Assignment operator | ||
| */ | ||
| ParamFunctorTempl & operator = (const ParamFunctorTempl & rhs) { | ||
| // ParamFunctor copy(rhs); | ||
| // swap unique_ptr by hand | ||
| // Impl * p = fImpl.release(); | ||
| // fImpl.reset(copy.fImpl.release()); | ||
| // copy.fImpl.reset(p); | ||
|
|
||
| if(this != &rhs) { | ||
| if (fImpl) delete fImpl; | ||
| fImpl = nullptr; | ||
| if (rhs.fImpl) | ||
| fImpl = rhs.fImpl->Clone(); | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| void * GetImpl() { return (void *) fImpl; } | ||
|
|
||
|
|
||
| T operator() ( T * x, double * p) { | ||
| return (*fImpl)(x,p); | ||
| } | ||
| // specialization used in TF1 | ||
| typedef T (*FreeFunc)(T *, double *); | ||
| ParamFunctorTempl(FreeFunc f) : fFunc{Adapt(f)} {} |
There was a problem hiding this comment.
Could these two not be unified into one, using Adapt?
What is the reason for using SFINAE to exclude them being the same type?
There was a problem hiding this comment.
The SFINAE was guarding against the by-value ParamFunctorTempl(Func f) taking priority over the copy constructor for a non-const lvalue. But anyway that's not needed anymore, because I have removed the FreeFunc constructor, as it was redundant like you suspected.
By the way, a side note before you also bring up that the std::function constructor is redundant: from the C++ perspective yes, but it has to stay because cppyy depends on that. The callbacks into Python only work via std::function arguments.
Test Results 23 files 23 suites 3d 18h 37m 35s ⏱️ For more details on these failures, see this check. Results for commit efb4d48. ♻️ This comment has been updated with latest results. |
`ParamFunctor` was still carrying the hand-rolled type erasure that the other `ROOT::Math` functors got rid of in 6c68bbd and a24465f: a `ParamFunctionBase` interface, `ParamFunctorHandler` and `ParamMemFunHandler` implementations of it, three `FuncEvaluator` partial specialisations to tell pointer types apart, a manual `Clone()`, a raw owning `Impl *` with hand-written copy constructor, assignment operator and destructor, and about 40 lines of commented-out code. All of that is what `std::function` does, and the class already had a `std::function` constructor sitting next to it. Store a single `std::function<T(const T *, const double *)>` instead and let the compiler generate the copy operations. The three callable shapes the `FuncEvaluator` specialisations used to dispatch on are kept by normalising them in one `Adapt()` helper: a callable taking const pointers is stored as is, a callable insisting on non-const pointers (the classic `T (T *x, double *p)` signature) gets them cast for it, and a pointer to a callable object is called through without taking ownership of it. That makes the separate `FreeFunc` constructor redundant, since `Adapt()` already normalises a free function pointer, so it goes. Nothing in ROOT converted a free function to a `ParamFunctor` implicitly. The `std::function` constructor stays implicit, on the other hand, because PyROOT needs it: cppyy binds a Python-side callable to the `TF1(const char *, ROOT::Math::ParamFunctor, ...)` overload through that conversion, and `tutorials/math/fit/fitNormSum.py` fails to find a viable overload without it. Calling a `ParamFunctor` is unchanged, and so is constructing one, with one further exception: the constructor from an object and one of its member functions now takes a plain `Obj *` rather than a `const PtrObj &` that only had to be dereferenceable. Every caller passes a raw pointer, and spelling that out rejects at the signature what used to fail inside the handler. The removed `GetImpl()` and `SetFunction()` were only handles on the deleted `ParamFunctionBase` and had no callers. 🤖 Done with the help of AI
3b30a35 to
efb4d48
Compare
2 participants
ParamFunctorwas still carrying the hand-rolled type erasure that the otherROOT::Mathfunctors got rid of in 6c68bbd and a24465f: aParamFunctionBaseinterface,ParamFunctorHandlerandParamMemFunHandlerimplementations of it, threeFuncEvaluatorpartial specialisations to tell pointer types apart, a manualClone(), a raw owningImpl *with hand-written copy constructor, assignment operator and destructor, and about 40 lines of commented-out code.All of that is what
std::functiondoes, and the class already had astd::functionconstructor sitting next to it. Store a singlestd::function<T(const T *, const double *)>instead and let the compiler generate the copy operations.The three callable shapes the
FuncEvaluatorspecialisations used to dispatch on are kept by normalising them in oneAdapt()helper: a callable taking const pointers is stored as is, a callable insisting on non-const pointers (the classicT (T *x, double *p)signature) gets them cast for it, and a pointer to a callable object is called through without taking ownership of it.Constructing and calling a
ParamFunctoris unchanged. The removedGetImpl()andSetFunction()were only handles on the deletedParamFunctionBaseand had no callers.The
<iostream>include went away with the code that needed it; two files that were picking it up transitively viaTF1.hnow include it themselves.🤖 Done with the help of AI