Modernize CostFunction and FirstOrderFunction constructors - Add constructors taking std::unique_ptr and ceres::Ownership to AutoDiffCostFunction, NumericDiffCostFunction, and their dynamic and first-order counterparts. - Standardize delegating constructor style to use parentheses. - Standardize ownership check in destructors. - Fix documentation typos and example code in headers. - Add comprehensive tests for Ownership and unique_ptr constructors. Change-Id: I573abd695cdd89905997b573620e8d99d1cacca2
diff --git a/include/ceres/autodiff_cost_function.h b/include/ceres/autodiff_cost_function.h index 4ca8c4e..667b385 100644 --- a/include/ceres/autodiff_cost_function.h +++ b/include/ceres/autodiff_cost_function.h
@@ -159,30 +159,38 @@ // Takes ownership of functor by default. Uses the template-provided // value for the number of residuals ("kNumResiduals"). explicit AutoDiffCostFunction(std::unique_ptr<CostFunctor> functor) - : AutoDiffCostFunction{std::move(functor), TAKE_OWNERSHIP, FIXED_INIT} {} + : AutoDiffCostFunction(std::move(functor), TAKE_OWNERSHIP) { + static_assert(kNumResiduals != DYNAMIC, + "When kNumResiduals is DYNAMIC, the number of residuals must " + "be provided as a constructor argument."); + } // Constructs the CostFunctor on the heap and takes the ownership. // Invocable only if the number of residuals is known at compile-time. - template <class... Args, - bool kIsDynamic = kNumResiduals == DYNAMIC, - std::enable_if_t<!kIsDynamic && - std::is_constructible_v<CostFunctor, Args&&...>>* = - nullptr> + template <typename... Args, + typename = std::enable_if_t< + (kNumResiduals != DYNAMIC) && + std::is_constructible_v<CostFunctor, Args&&...>>> explicit AutoDiffCostFunction(Args&&... args) // NOTE We explicitly use direct initialization using parentheses instead // of uniform initialization using braces to avoid narrowing conversion // warnings. - : AutoDiffCostFunction{ - std::make_unique<CostFunctor>(std::forward<Args>(args)...)} {} + : AutoDiffCostFunction( + std::make_unique<CostFunctor>(std::forward<Args>(args)...), + TAKE_OWNERSHIP) {} + // Takes ownership of functor by default. Ignores the template-provided + // kNumResiduals in favor of the "num_residuals" argument provided. AutoDiffCostFunction(std::unique_ptr<CostFunctor> functor, int num_residuals) - : AutoDiffCostFunction{ - std::move(functor), num_residuals, TAKE_OWNERSHIP, DYNAMIC_INIT} {} + : AutoDiffCostFunction(std::move(functor), num_residuals, TAKE_OWNERSHIP) {} explicit AutoDiffCostFunction(CostFunctor* functor, Ownership ownership = TAKE_OWNERSHIP) - : AutoDiffCostFunction{ - std::unique_ptr<CostFunctor>{functor}, ownership, FIXED_INIT} {} + : AutoDiffCostFunction(std::unique_ptr<CostFunctor>(functor), ownership) { + static_assert(kNumResiduals != DYNAMIC, + "When kNumResiduals is DYNAMIC, the number of residuals must " + "be provided as a constructor argument."); + } // Takes ownership of functor by default. Ignores the template-provided // kNumResiduals in favor of the "num_residuals" argument provided. @@ -192,10 +200,8 @@ AutoDiffCostFunction(CostFunctor* functor, int num_residuals, Ownership ownership = TAKE_OWNERSHIP) - : AutoDiffCostFunction{std::unique_ptr<CostFunctor>{functor}, - num_residuals, - ownership, - DYNAMIC_INIT} {} + : AutoDiffCostFunction( + std::unique_ptr<CostFunctor>(functor), num_residuals, ownership) {} AutoDiffCostFunction(AutoDiffCostFunction&& other) noexcept = default; AutoDiffCostFunction& operator=(AutoDiffCostFunction&& other) noexcept = @@ -235,38 +241,27 @@ this->num_residuals(), residuals, jacobians); - }; + } const CostFunctor& functor() const { return *functor_; } private: - // Tags used to differentiate between dynamic and fixed size constructor - // delegate invocations. - static constexpr std::integral_constant<int, DYNAMIC> DYNAMIC_INIT{}; - static constexpr std::integral_constant<int, kNumResiduals> FIXED_INIT{}; + // Internal delegating constructor for fixed-size residuals. + AutoDiffCostFunction(std::unique_ptr<CostFunctor> functor, Ownership ownership) + : functor_(std::move(functor)), ownership_(ownership) {} - template <class InitTag> + // Internal delegating constructor for dynamic-size residuals. AutoDiffCostFunction(std::unique_ptr<CostFunctor> functor, int num_residuals, - Ownership ownership, - InitTag /*unused*/) - : functor_{std::move(functor)}, ownership_{ownership} { - static_assert(kNumResiduals == FIXED_INIT, - "Can't run the fixed-size constructor if the number of " - "residuals is set to ceres::DYNAMIC."); - - if constexpr (InitTag::value == DYNAMIC_INIT) { + Ownership ownership) + : functor_(std::move(functor)), ownership_(ownership) { + if constexpr (kNumResiduals == DYNAMIC) { this->set_num_residuals(num_residuals); + } else { + DCHECK_EQ(num_residuals, kNumResiduals); } } - template <class InitTag> - AutoDiffCostFunction(std::unique_ptr<CostFunctor> functor, - Ownership ownership, - InitTag tag) - : AutoDiffCostFunction{ - std::move(functor), kNumResiduals, ownership, tag} {} - std::unique_ptr<CostFunctor> functor_; Ownership ownership_; };
diff --git a/include/ceres/numeric_diff_cost_function.h b/include/ceres/numeric_diff_cost_function.h index 5b102ae..1822422 100644 --- a/include/ceres/numeric_diff_cost_function.h +++ b/include/ceres/numeric_diff_cost_function.h
@@ -186,32 +186,41 @@ Ownership ownership = TAKE_OWNERSHIP, int num_residuals = kNumResiduals, const NumericDiffOptions& options = NumericDiffOptions()) - : NumericDiffCostFunction{std::unique_ptr<CostFunctor>{functor}, + : NumericDiffCostFunction(std::unique_ptr<CostFunctor>(functor), ownership, num_residuals, - options} {} + options) { + if constexpr (kNumResiduals != DYNAMIC) { + DCHECK_EQ(num_residuals, kNumResiduals); + } + } explicit NumericDiffCostFunction( std::unique_ptr<CostFunctor> functor, int num_residuals = kNumResiduals, const NumericDiffOptions& options = NumericDiffOptions()) - : NumericDiffCostFunction{ - std::move(functor), TAKE_OWNERSHIP, num_residuals, options} {} + : NumericDiffCostFunction( + std::move(functor), TAKE_OWNERSHIP, num_residuals, options) { + if constexpr (kNumResiduals != DYNAMIC) { + DCHECK_EQ(num_residuals, kNumResiduals); + } + } // Constructs the CostFunctor on the heap and takes the ownership. // Invocable only if the number of residuals is known at compile-time. template <class... Args, - bool kIsDynamic = kNumResiduals == DYNAMIC, - std::enable_if_t<!kIsDynamic && - std::is_constructible_v<CostFunctor, Args&&...>>* = - nullptr> + typename = std::enable_if_t< + (kNumResiduals != DYNAMIC) && + std::is_constructible_v<CostFunctor, Args&&...>>> explicit NumericDiffCostFunction(Args&&... args) // NOTE We explicitly use direct initialization using parentheses instead // of uniform initialization using braces to avoid narrowing conversion // warnings. - : NumericDiffCostFunction{ + : NumericDiffCostFunction( std::make_unique<CostFunctor>(std::forward<Args>(args)...), - TAKE_OWNERSHIP} {} + TAKE_OWNERSHIP, + kNumResiduals, + NumericDiffOptions()) {} NumericDiffCostFunction(NumericDiffCostFunction&& other) noexcept = default; NumericDiffCostFunction& operator=(NumericDiffCostFunction&& other) noexcept = @@ -220,7 +229,7 @@ NumericDiffCostFunction& operator=(const NumericDiffCostFunction&) = delete; ~NumericDiffCostFunction() override { - if (ownership_ != TAKE_OWNERSHIP) { + if (ownership_ == DO_NOT_TAKE_OWNERSHIP) { functor_.release(); } } @@ -229,7 +238,6 @@ double* residuals, double** jacobians) const override { using absl::FixedArray; - using internal::NumericDiff; using ParameterDims = typename SizedCostFunction<kNumResiduals, Ns...>::ParameterDims; @@ -275,11 +283,13 @@ private: explicit NumericDiffCostFunction(std::unique_ptr<CostFunctor> functor, Ownership ownership, - [[maybe_unused]] int num_residuals, + int num_residuals, const NumericDiffOptions& options) : functor_(std::move(functor)), ownership_(ownership), options_(options) { if constexpr (kNumResiduals == DYNAMIC) { this->set_num_residuals(num_residuals); + } else { + DCHECK_EQ(num_residuals, kNumResiduals); } }
diff --git a/include/ceres/numeric_diff_first_order_function.h b/include/ceres/numeric_diff_first_order_function.h index fbcb9a3..f136e31 100644 --- a/include/ceres/numeric_diff_first_order_function.h +++ b/include/ceres/numeric_diff_first_order_function.h
@@ -131,19 +131,30 @@ explicit NumericDiffFirstOrderFunction( std::unique_ptr<FirstOrderFunctor> functor, const NumericDiffOptions& options = NumericDiffOptions()) - : NumericDiffFirstOrderFunction{std::move(functor), - kNumParameters, - TAKE_OWNERSHIP, - options, - FIXED_INIT} {} + : NumericDiffFirstOrderFunction( + std::move(functor), TAKE_OWNERSHIP, kNumParameters, options) { + static_assert(kNumParameters != DYNAMIC, + "When kNumParameters is DYNAMIC, the number of parameters " + "must be provided as a constructor argument."); + } + template <class... Args, - bool kIsDynamic = kNumParameters == DYNAMIC, - std::enable_if_t<!kIsDynamic && - std::is_constructible_v<FirstOrderFunctor, - Args&&...>>* = nullptr> + typename = std::enable_if_t< + (kNumParameters != DYNAMIC) && + std::is_constructible_v<FirstOrderFunctor, Args&&...>>> explicit NumericDiffFirstOrderFunction(Args&&... args) - : NumericDiffFirstOrderFunction{ - std::make_unique<FirstOrderFunctor>(std::forward<Args>(args)...)} {} + : NumericDiffFirstOrderFunction( + std::make_unique<FirstOrderFunctor>(std::forward<Args>(args)...)) {} + + explicit NumericDiffFirstOrderFunction(FirstOrderFunctor* functor, + Ownership ownership = TAKE_OWNERSHIP) + : NumericDiffFirstOrderFunction(std::unique_ptr<FirstOrderFunctor>(functor), + kNumParameters, + ownership) { + static_assert(kNumParameters != DYNAMIC, + "When kNumParameters is DYNAMIC, the number of parameters " + "must be provided as a constructor argument."); + } // Constructor for the case where the parameter size is specified at run time. explicit NumericDiffFirstOrderFunction( @@ -151,14 +162,15 @@ int num_parameters, Ownership ownership = TAKE_OWNERSHIP, const NumericDiffOptions& options = NumericDiffOptions()) - : NumericDiffFirstOrderFunction{std::move(functor), - num_parameters, - ownership, - options, - DYNAMIC_INIT} {} + : NumericDiffFirstOrderFunction( + std::move(functor), ownership, num_parameters, options) { + if constexpr (kNumParameters != DYNAMIC) { + DCHECK_EQ(num_parameters, kNumParameters); + } + } ~NumericDiffFirstOrderFunction() override { - if (ownership_ != TAKE_OWNERSHIP) { + if (ownership_ == DO_NOT_TAKE_OWNERSHIP) { functor_.release(); } } @@ -213,29 +225,18 @@ const FirstOrderFunctor& functor() const { return *functor_; } private: - // Tags used to differentiate between dynamic and fixed size constructor - // delegate invocations. - static constexpr std::integral_constant<int, DYNAMIC> DYNAMIC_INIT{}; - static constexpr std::integral_constant<int, kNumParameters> FIXED_INIT{}; - - template <class InitTag> - explicit NumericDiffFirstOrderFunction( - std::unique_ptr<FirstOrderFunctor> functor, - int num_parameters, - Ownership ownership, - const NumericDiffOptions& options, - InitTag /*unused*/) + explicit NumericDiffFirstOrderFunction(std::unique_ptr<FirstOrderFunctor> functor, + Ownership ownership, + int num_parameters, + const NumericDiffOptions& options) : functor_(std::move(functor)), num_parameters_(num_parameters), ownership_(ownership), options_(options) { - static_assert( - kNumParameters == FIXED_INIT, - "Template parameter must be DYNAMIC when using this constructor. If " - "you want to provide the number of parameters statically use the other " - "constructor."); - if constexpr (InitTag::value == DYNAMIC_INIT) { - CHECK_GT(num_parameters, 0); + if constexpr (kNumParameters == DYNAMIC) { + DCHECK_GT(num_parameters, 0); + } else { + DCHECK_EQ(num_parameters, kNumParameters); } }
diff --git a/internal/ceres/autodiff_cost_function_test.cc b/internal/ceres/autodiff_cost_function_test.cc index dca67ba..18900ef 100644 --- a/internal/ceres/autodiff_cost_function_test.cc +++ b/internal/ceres/autodiff_cost_function_test.cc
@@ -92,6 +92,30 @@ delete cost_function; } +TEST(AutodiffCostFunction, OwnershipTest) { + BinaryScalarCost functor(1.0); + { + AutoDiffCostFunction<BinaryScalarCost, 1, 2, 2> cost_function( + &functor, DO_NOT_TAKE_OWNERSHIP); + double parameters_data[4] = {1.0, 2.0, 3.0, 4.0}; + double* parameters[2] = {parameters_data, parameters_data + 2}; + double residuals; + cost_function.Evaluate(parameters, &residuals, nullptr); + EXPECT_EQ(residuals, 10.0); + } +} + +TEST(AutodiffCostFunction, UniquePtrTest) { + auto cost_function = + std::make_unique<AutoDiffCostFunction<BinaryScalarCost, 1, 2, 2>>( + std::make_unique<BinaryScalarCost>(1.0)); + double parameters_data[4] = {1.0, 2.0, 3.0, 4.0}; + double* parameters[2] = {parameters_data, parameters_data + 2}; + double residuals; + cost_function->Evaluate(parameters, &residuals, nullptr); + EXPECT_EQ(residuals, 10.0); +} + struct TenParameterCost { template <typename T> bool operator()(const T* const x0,
diff --git a/internal/ceres/autodiff_first_order_function_test.cc b/internal/ceres/autodiff_first_order_function_test.cc index 5f1eb0b..a75f0e7 100644 --- a/internal/ceres/autodiff_first_order_function_test.cc +++ b/internal/ceres/autodiff_first_order_function_test.cc
@@ -73,5 +73,20 @@ EXPECT_EQ(gradient[3], parameters[2]); } +TEST(AutoDiffFirstOrderFunction, OwnershipTest) { + QuadraticCostFunctor functor(1.0); + { + AutoDiffFirstOrderFunction<QuadraticCostFunctor, 4> function( + &functor, DO_NOT_TAKE_OWNERSHIP); + double parameters[4] = {1.0, 2.0, 3.0, 4.0}; + double cost; + function.Evaluate(parameters, &cost, nullptr); + EXPECT_EQ(cost, 13.0); + } + // If ownership was taken, this would be a use-after-free or double-free + // (though here it's on stack, so it would just be wrong). + // The test is that it doesn't crash during destruction of 'function'. +} + } // namespace internal } // namespace ceres
diff --git a/internal/ceres/dynamic_autodiff_cost_function_test.cc b/internal/ceres/dynamic_autodiff_cost_function_test.cc index 5b7261c..019eb79 100644 --- a/internal/ceres/dynamic_autodiff_cost_function_test.cc +++ b/internal/ceres/dynamic_autodiff_cost_function_test.cc
@@ -820,4 +820,21 @@ (void)DynamicAutoDiffCostFunction(std::make_unique<MyCostFunctor>()); } +TEST(DynamicAutoDiffCostFunctionTest, Ownership) { + MyCostFunctor functor; + { + DynamicAutoDiffCostFunction<MyCostFunctor> cost_function( + &functor, DO_NOT_TAKE_OWNERSHIP); + cost_function.AddParameterBlock(3); + cost_function.SetNumResiduals(3); + } +} + +TEST(DynamicAutoDiffCostFunctionTest, ExplicitUniquePtr) { + auto functor = std::make_unique<MyCostFunctor>(); + DynamicAutoDiffCostFunction<MyCostFunctor> cost_function(std::move(functor)); + cost_function.AddParameterBlock(3); + cost_function.SetNumResiduals(3); +} + } // namespace ceres::internal
diff --git a/internal/ceres/dynamic_numeric_diff_cost_function_test.cc b/internal/ceres/dynamic_numeric_diff_cost_function_test.cc index aec7819..b4d7b73 100644 --- a/internal/ceres/dynamic_numeric_diff_cost_function_test.cc +++ b/internal/ceres/dynamic_numeric_diff_cost_function_test.cc
@@ -525,9 +525,19 @@ (void)DynamicNumericDiffCostFunction<MyCostFunctor>(); } -TEST(DynamicAutoDiffCostFunctionTest, UniquePtr) { +TEST(DynamicNumericDiffCostFunctionTest, UniquePtr) { (void)DynamicNumericDiffCostFunction<MyCostFunctor>( std::make_unique<MyCostFunctor>()); } +TEST(DynamicNumericDiffCostFunctionTest, Ownership) { + MyCostFunctor functor; + { + DynamicNumericDiffCostFunction<MyCostFunctor> cost_function( + &functor, DO_NOT_TAKE_OWNERSHIP); + cost_function.AddParameterBlock(3); + cost_function.SetNumResiduals(3); + } +} + } // namespace ceres::internal
diff --git a/internal/ceres/numeric_diff_cost_function_test.cc b/internal/ceres/numeric_diff_cost_function_test.cc index 235c266..0b393d7 100644 --- a/internal/ceres/numeric_diff_cost_function_test.cc +++ b/internal/ceres/numeric_diff_cost_function_test.cc
@@ -460,4 +460,16 @@ NumericDiffCostFunction<EasyFunctor, CENTRAL, 3, 5, 5>>(); } +TEST(NumericDiffCostFunction, Ownership) { + EasyFunctor functor; + { + NumericDiffCostFunction<EasyFunctor, CENTRAL, 3, 5, 5> cost_function( + &functor, DO_NOT_TAKE_OWNERSHIP); + double parameters_data[10] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + double* parameters[2] = {parameters_data, parameters_data + 5}; + double residuals[3]; + cost_function.Evaluate(parameters, residuals, nullptr); + } +} + } // namespace ceres::internal
diff --git a/internal/ceres/numeric_diff_first_order_function_test.cc b/internal/ceres/numeric_diff_first_order_function_test.cc index dc02b13..bf5a394 100644 --- a/internal/ceres/numeric_diff_first_order_function_test.cc +++ b/internal/ceres/numeric_diff_first_order_function_test.cc
@@ -98,4 +98,16 @@ EXPECT_NEAR(gradient[3], parameters[2], kTolerance); } +TEST(NumericDiffFirstOrderFunction, OwnershipTest) { + QuadraticCostFunctor functor(1.0); + { + NumericDiffFirstOrderFunction<QuadraticCostFunctor, CENTRAL, 4> function( + &functor, DO_NOT_TAKE_OWNERSHIP); + double parameters[4] = {1.0, 2.0, 3.0, 4.0}; + double cost; + function.Evaluate(parameters, &cost, nullptr); + EXPECT_EQ(cost, 13.0); + } +} + } // namespace ceres::internal