[modernize] Modernize AutoDiff and NumericDiff CostFunctions to C++17 Modernize the construction and initialization logic of AutoDiffCostFunction, NumericDiffCostFunction, AutoDiffFirstOrderFunction, NumericDiffFirstOrderFunction, DynamicAutoDiffCostFunction, DynamicNumericDiffCostFunction, and CostFunctionToFunctor by utilizing C++17 features: - Simplify constructor delegation using if constexpr, removing the need for internal InitTag structures. - Use std::is_constructible_v for cleaner SFINAE in variadic constructors. - Add static_assert to catch improper usage of DYNAMIC residuals/parameters at compile-time. - Add Ownership support to AutoDiffFirstOrderFunction, DynamicAutoDiffCostFunction, and DynamicNumericDiffCostFunction. - Simplify CostFunctionToFunctor::operator() using variadic parameter packs. - Use this->num_residuals() for consistent and clear access to base class state. - Minor code quality improvements and removal of redundant template parameters. Change-Id: Id760a26ba2ceca4fb2939ef7632f2f1d9cc430c9
diff --git a/include/ceres/autodiff_cost_function.h b/include/ceres/autodiff_cost_function.h index 878b2ec..4ca8c4e 100644 --- a/include/ceres/autodiff_cost_function.h +++ b/include/ceres/autodiff_cost_function.h
@@ -232,7 +232,7 @@ return internal::AutoDifferentiate<kNumResiduals, ParameterDims>( *functor_, parameters, - SizedCostFunction<kNumResiduals, Ns...>::num_residuals(), + this->num_residuals(), residuals, jacobians); }; @@ -256,7 +256,7 @@ "residuals is set to ceres::DYNAMIC."); if constexpr (InitTag::value == DYNAMIC_INIT) { - SizedCostFunction<kNumResiduals, Ns...>::set_num_residuals(num_residuals); + this->set_num_residuals(num_residuals); } }
diff --git a/include/ceres/autodiff_first_order_function.h b/include/ceres/autodiff_first_order_function.h index 0e7def3..d29c8d6 100644 --- a/include/ceres/autodiff_first_order_function.h +++ b/include/ceres/autodiff_first_order_function.h
@@ -91,7 +91,7 @@ // // FirstOrderFunction* function = // new AutoDiffFirstOrderFunction<QuadraticCostFunctor, 4>( -// std::make_unique<QuadraticCostFunctor>(1.0))); +// std::make_unique<QuadraticCostFunctor>(1.0)); // // In the instantiation above, the template parameters following // "QuadraticCostFunctor", "4", describe the functor as computing a @@ -105,6 +105,32 @@ template <typename FirstOrderFunctor, int kNumParameters> class AutoDiffFirstOrderFunction final : public FirstOrderFunction { public: + // Takes ownership of functor by default. + explicit AutoDiffFirstOrderFunction( + std::unique_ptr<FirstOrderFunctor> functor) + : AutoDiffFirstOrderFunction(std::move(functor), TAKE_OWNERSHIP) { + static_assert(kNumParameters > 0, "kNumParameters must be positive"); + } + + // Constructs the FirstOrderFunctor on the heap and takes the ownership. + template <typename... Args, + typename = std::enable_if_t< + std::is_constructible_v<FirstOrderFunctor, Args&&...>>> + explicit AutoDiffFirstOrderFunction(Args&&... args) + // NOTE We explicitly use direct initialization using parentheses instead + // of uniform initialization using braces to avoid narrowing conversion + // warnings. + : AutoDiffFirstOrderFunction( + std::make_unique<FirstOrderFunctor>(std::forward<Args>(args)...), + TAKE_OWNERSHIP) {} + + explicit AutoDiffFirstOrderFunction(FirstOrderFunctor* functor, + Ownership ownership = TAKE_OWNERSHIP) + : AutoDiffFirstOrderFunction(std::unique_ptr<FirstOrderFunctor>(functor), + ownership) { + static_assert(kNumParameters > 0, "kNumParameters must be positive"); + } + AutoDiffFirstOrderFunction(const AutoDiffFirstOrderFunction&) = delete; AutoDiffFirstOrderFunction& operator=(const AutoDiffFirstOrderFunction&) = delete; @@ -113,19 +139,12 @@ AutoDiffFirstOrderFunction& operator=( AutoDiffFirstOrderFunction&& other) noexcept = default; - explicit AutoDiffFirstOrderFunction( - std::unique_ptr<FirstOrderFunctor> functor) - : functor_(std::move(functor)) { - static_assert(kNumParameters > 0, "kNumParameters must be positive"); + ~AutoDiffFirstOrderFunction() override { + if (ownership_ == DO_NOT_TAKE_OWNERSHIP) { + functor_.release(); + } } - template <class... Args, - std::enable_if_t<std::is_constructible_v<FirstOrderFunctor, - Args&&...>>* = nullptr> - explicit AutoDiffFirstOrderFunction(Args&&... args) - : AutoDiffFirstOrderFunction{ - std::make_unique<FirstOrderFunctor>(std::forward<Args>(args)...)} {} - bool Evaluate(const double* const parameters, double* cost, double* gradient) const override { @@ -159,7 +178,12 @@ const FirstOrderFunctor& functor() const { return *functor_; } private: + AutoDiffFirstOrderFunction(std::unique_ptr<FirstOrderFunctor> functor, + Ownership ownership) + : functor_(std::move(functor)), ownership_(ownership) {} + std::unique_ptr<FirstOrderFunctor> functor_; + Ownership ownership_; }; } // namespace ceres
diff --git a/include/ceres/cost_function_to_functor.h b/include/ceres/cost_function_to_functor.h index a2ca6d8..9505159 100644 --- a/include/ceres/cost_function_to_functor.h +++ b/include/ceres/cost_function_to_functor.h
@@ -134,37 +134,20 @@ template <typename T, typename... Ts> bool operator()(const T* p1, Ts*... ps) const { - // Add one because of residual block. static_assert(sizeof...(Ts) + 1 == ParameterDims::kNumParameterBlocks + 1, "Invalid number of parameter blocks specified."); - auto params = std::make_tuple(p1, ps...); - - // Extract residual pointer from params. The residual pointer is the - // last pointer. - constexpr int kResidualIndex = ParameterDims::kNumParameterBlocks; - T* residuals = std::get<kResidualIndex>(params); - - // Extract parameter block pointers from params. - using Indices = - std::make_integer_sequence<int, ParameterDims::kNumParameterBlocks>; - std::array<const T*, ParameterDims::kNumParameterBlocks> parameter_blocks = - GetParameterPointers<T>(params, Indices()); - - return cost_functor_(parameter_blocks.data(), residuals); + return [this](const auto&... pointers) { + const std::array<const T*, ParameterDims::kNumParameterBlocks + 1> + params{{pointers...}}; + T* residuals = const_cast<T*>(params[ParameterDims::kNumParameterBlocks]); + return cost_functor_(params.data(), residuals); + }(p1, ps...); } private: using ParameterDims = internal::StaticParameterDims<Ns...>; - template <typename T, typename Tuple, int... Indices> - static std::array<const T*, ParameterDims::kNumParameterBlocks> - GetParameterPointers(const Tuple& paramPointers, - std::integer_sequence<int, Indices...>) { - return std::array<const T*, ParameterDims::kNumParameterBlocks>{ - {std::get<Indices>(paramPointers)...}}; - } - DynamicCostFunctionToFunctor cost_functor_; };
diff --git a/include/ceres/dynamic_autodiff_cost_function.h b/include/ceres/dynamic_autodiff_cost_function.h index 7efdad0..05b13bc 100644 --- a/include/ceres/dynamic_autodiff_cost_function.h +++ b/include/ceres/dynamic_autodiff_cost_function.h
@@ -79,25 +79,31 @@ template <typename CostFunctor, int Stride = 4> class DynamicAutoDiffCostFunction final : public DynamicCostFunction { public: + // Takes ownership of functor by default. + explicit DynamicAutoDiffCostFunction(std::unique_ptr<CostFunctor> functor) + : DynamicAutoDiffCostFunction(std::move(functor), TAKE_OWNERSHIP) { + static_assert(Stride > 0, "Stride must be positive"); + } + // Constructs the CostFunctor on the heap and takes the ownership. - template <class... Args, - std::enable_if_t<std::is_constructible_v<CostFunctor, Args&&...>>* = - nullptr> + template <typename... Args, + typename = std::enable_if_t< + std::is_constructible_v<CostFunctor, Args&&...>>> explicit DynamicAutoDiffCostFunction(Args&&... args) // NOTE We explicitly use direct initialization using parentheses instead // of uniform initialization using braces to avoid narrowing conversion // warnings. - : DynamicAutoDiffCostFunction{ - std::make_unique<CostFunctor>(std::forward<Args>(args)...)} {} + : DynamicAutoDiffCostFunction( + std::make_unique<CostFunctor>(std::forward<Args>(args)...), + TAKE_OWNERSHIP) {} // Takes ownership by default. explicit DynamicAutoDiffCostFunction(CostFunctor* functor, Ownership ownership = TAKE_OWNERSHIP) - : DynamicAutoDiffCostFunction{std::unique_ptr<CostFunctor>{functor}, - ownership} {} - - explicit DynamicAutoDiffCostFunction(std::unique_ptr<CostFunctor> functor) - : DynamicAutoDiffCostFunction{std::move(functor), TAKE_OWNERSHIP} {} + : DynamicAutoDiffCostFunction(std::unique_ptr<CostFunctor>(functor), + ownership) { + static_assert(Stride > 0, "Stride must be positive"); + } DynamicAutoDiffCostFunction(const DynamicAutoDiffCostFunction& other) = delete;
diff --git a/include/ceres/dynamic_numeric_diff_cost_function.h b/include/ceres/dynamic_numeric_diff_cost_function.h index 2cd0e9c..55b7913 100644 --- a/include/ceres/dynamic_numeric_diff_cost_function.h +++ b/include/ceres/dynamic_numeric_diff_cost_function.h
@@ -72,36 +72,39 @@ // also specify the sizes after creating the // DynamicNumericDiffCostFunction. For example: // -// DynamicAutoDiffCostFunction<MyCostFunctor, CENTRAL> cost_function; +// DynamicNumericDiffCostFunction<MyCostFunctor, CENTRAL> cost_function; // cost_function.AddParameterBlock(5); // cost_function.AddParameterBlock(10); // cost_function.SetNumResiduals(21); template <typename CostFunctor, NumericDiffMethodType kMethod = CENTRAL> class DynamicNumericDiffCostFunction final : public DynamicCostFunction { public: - explicit DynamicNumericDiffCostFunction( - const CostFunctor* functor, - Ownership ownership = TAKE_OWNERSHIP, - const NumericDiffOptions& options = NumericDiffOptions()) - : DynamicNumericDiffCostFunction{ - std::unique_ptr<const CostFunctor>{functor}, ownership, options} {} - + // Takes ownership of functor by default. explicit DynamicNumericDiffCostFunction( std::unique_ptr<const CostFunctor> functor, const NumericDiffOptions& options = NumericDiffOptions()) - : DynamicNumericDiffCostFunction{ - std::move(functor), TAKE_OWNERSHIP, options} {} + : DynamicNumericDiffCostFunction( + std::move(functor), TAKE_OWNERSHIP, options) {} // Constructs the CostFunctor on the heap and takes the ownership. template <class... Args, - std::enable_if_t<std::is_constructible_v<CostFunctor, Args&&...>>* = - nullptr> + typename = std::enable_if_t<std::is_constructible_v<CostFunctor, + Args&&...>>> explicit DynamicNumericDiffCostFunction(Args&&... args) // NOTE We explicitly use direct initialization using parentheses instead // of uniform initialization using braces to avoid narrowing conversion // warnings. - : DynamicNumericDiffCostFunction{ - std::make_unique<CostFunctor>(std::forward<Args>(args)...)} {} + : DynamicNumericDiffCostFunction( + std::make_unique<const CostFunctor>(std::forward<Args>(args)...), + TAKE_OWNERSHIP, + NumericDiffOptions()) {} + + explicit DynamicNumericDiffCostFunction( + const CostFunctor* functor, + Ownership ownership = TAKE_OWNERSHIP, + const NumericDiffOptions& options = NumericDiffOptions()) + : DynamicNumericDiffCostFunction( + std::unique_ptr<const CostFunctor>(functor), ownership, options) {} DynamicNumericDiffCostFunction(const DynamicNumericDiffCostFunction&) = delete; @@ -113,7 +116,7 @@ DynamicNumericDiffCostFunction&& other) noexcept = default; ~DynamicNumericDiffCostFunction() override { - if (ownership_ != TAKE_OWNERSHIP) { + if (ownership_ == DO_NOT_TAKE_OWNERSHIP) { functor_.release(); } } @@ -220,4 +223,4 @@ } // namespace ceres -#endif // CERES_PUBLIC_DYNAMIC_AUTODIFF_COST_FUNCTION_H_ +#endif // CERES_PUBLIC_DYNAMIC_NUMERIC_DIFF_COST_FUNCTION_H_
diff --git a/include/ceres/numeric_diff_cost_function.h b/include/ceres/numeric_diff_cost_function.h index 47e4af4..5b102ae 100644 --- a/include/ceres/numeric_diff_cost_function.h +++ b/include/ceres/numeric_diff_cost_function.h
@@ -263,7 +263,7 @@ functor_.get(), residuals, options_, - SizedCostFunction<kNumResiduals, Ns...>::num_residuals(), + this->num_residuals(), parameters_reference_copy.data(), jacobians); @@ -279,7 +279,7 @@ const NumericDiffOptions& options) : functor_(std::move(functor)), ownership_(ownership), options_(options) { if constexpr (kNumResiduals == DYNAMIC) { - SizedCostFunction<kNumResiduals, Ns...>::set_num_residuals(num_residuals); + this->set_num_residuals(num_residuals); } }