[math] Use std::function in ROOT::Math::ParamFunctor - #23211
Conversation
`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. Constructing and calling a `ParamFunctor` is unchanged. The removed `GetImpl()` and `SetFunction()` were only handles on the deleted `ParamFunctionBase` and had no callers. The `<iostream>` include went away with the code that needed it; two files that were picking it up transitively via `TF1.h` now include it themselves. 🤖 Done with the help of AI
2867e5e to
3b30a35
Compare
| #include <cmath> | ||
| #include <cstdlib> | ||
| #include <cassert> | ||
| #include <ostream> |
There was a problem hiding this comment.
This and the others should really be in their own commit, placed before the work on the function interface.
There was a problem hiding this comment.
Good point! I'll change that.
| /// | ||
| /// \author Stefan Schmitt DESY, 14.10.2008 | ||
|
|
||
| #include <iostream> |
There was a problem hiding this comment.
Good practice to have the C++ includes last in the list. (Otherwise, this can hide the fact that ROOT's headers/sources are missing an include, similar to what was revealed in this PR).
| 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?
| // 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); } |
| 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.
| /// 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?
Test Results 23 files 23 suites 3d 13h 23m 35s ⏱️ For more details on these failures, see this check. Results for commit 3b30a35. |
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