diff --git a/include/ceres/autodiff_cost_function.h b/include/ceres/autodiff_cost_function.h index 4ca8c4e6b..667b385f3 100644 --- a/include/ceres/autodiff_cost_function.h +++ b/include/ceres/autodiff_cost_function.h @@ -159,30 +159,38 @@ class AutoDiffCostFunction final // Takes ownership of functor by default. Uses the template-provided // value for the number of residuals ("kNumResiduals"). explicit AutoDiffCostFunction(std::unique_ptr 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 >* = - nullptr> + template >> 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(std::forward(args)...)} {} + : AutoDiffCostFunction( + std::make_unique(std::forward(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 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{functor}, ownership, FIXED_INIT} {} + : AutoDiffCostFunction(std::unique_ptr(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 @@ class AutoDiffCostFunction final AutoDiffCostFunction(CostFunctor* functor, int num_residuals, Ownership ownership = TAKE_OWNERSHIP) - : AutoDiffCostFunction{std::unique_ptr{functor}, - num_residuals, - ownership, - DYNAMIC_INIT} {} + : AutoDiffCostFunction( + std::unique_ptr(functor), num_residuals, ownership) {} AutoDiffCostFunction(AutoDiffCostFunction&& other) noexcept = default; AutoDiffCostFunction& operator=(AutoDiffCostFunction&& other) noexcept = @@ -235,38 +241,27 @@ class AutoDiffCostFunction final 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 DYNAMIC_INIT{}; - static constexpr std::integral_constant FIXED_INIT{}; + // Internal delegating constructor for fixed-size residuals. + AutoDiffCostFunction(std::unique_ptr functor, Ownership ownership) + : functor_(std::move(functor)), ownership_(ownership) {} - template + // Internal delegating constructor for dynamic-size residuals. AutoDiffCostFunction(std::unique_ptr 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 - AutoDiffCostFunction(std::unique_ptr functor, - Ownership ownership, - InitTag tag) - : AutoDiffCostFunction{ - std::move(functor), kNumResiduals, ownership, tag} {} - std::unique_ptr functor_; Ownership ownership_; }; diff --git a/include/ceres/numeric_diff_cost_function.h b/include/ceres/numeric_diff_cost_function.h index 5b102aed7..182242267 100644 --- a/include/ceres/numeric_diff_cost_function.h +++ b/include/ceres/numeric_diff_cost_function.h @@ -186,32 +186,41 @@ class NumericDiffCostFunction final Ownership ownership = TAKE_OWNERSHIP, int num_residuals = kNumResiduals, const NumericDiffOptions& options = NumericDiffOptions()) - : NumericDiffCostFunction{std::unique_ptr{functor}, + : NumericDiffCostFunction(std::unique_ptr(functor), ownership, num_residuals, - options} {} + options) { + if constexpr (kNumResiduals != DYNAMIC) { + DCHECK_EQ(num_residuals, kNumResiduals); + } + } explicit NumericDiffCostFunction( std::unique_ptr 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 >* = - nullptr> + typename = std::enable_if_t< + (kNumResiduals != DYNAMIC) && + std::is_constructible_v>> 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(std::forward(args)...), - TAKE_OWNERSHIP} {} + TAKE_OWNERSHIP, + kNumResiduals, + NumericDiffOptions()) {} NumericDiffCostFunction(NumericDiffCostFunction&& other) noexcept = default; NumericDiffCostFunction& operator=(NumericDiffCostFunction&& other) noexcept = @@ -220,7 +229,7 @@ class NumericDiffCostFunction final NumericDiffCostFunction& operator=(const NumericDiffCostFunction&) = delete; ~NumericDiffCostFunction() override { - if (ownership_ != TAKE_OWNERSHIP) { + if (ownership_ == DO_NOT_TAKE_OWNERSHIP) { functor_.release(); } } @@ -229,7 +238,6 @@ class NumericDiffCostFunction final double* residuals, double** jacobians) const override { using absl::FixedArray; - using internal::NumericDiff; using ParameterDims = typename SizedCostFunction::ParameterDims; @@ -275,11 +283,13 @@ class NumericDiffCostFunction final private: explicit NumericDiffCostFunction(std::unique_ptr 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 fbcb9a347..f136e31a9 100644 --- a/include/ceres/numeric_diff_first_order_function.h +++ b/include/ceres/numeric_diff_first_order_function.h @@ -131,19 +131,30 @@ class NumericDiffFirstOrderFunction final : public FirstOrderFunction { explicit NumericDiffFirstOrderFunction( std::unique_ptr 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 >* = nullptr> + typename = std::enable_if_t< + (kNumParameters != DYNAMIC) && + std::is_constructible_v>> explicit NumericDiffFirstOrderFunction(Args&&... args) - : NumericDiffFirstOrderFunction{ - std::make_unique(std::forward(args)...)} {} + : NumericDiffFirstOrderFunction( + std::make_unique(std::forward(args)...)) {} + + explicit NumericDiffFirstOrderFunction(FirstOrderFunctor* functor, + Ownership ownership = TAKE_OWNERSHIP) + : NumericDiffFirstOrderFunction(std::unique_ptr(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 @@ class NumericDiffFirstOrderFunction final : public FirstOrderFunction { 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 @@ class NumericDiffFirstOrderFunction final : public FirstOrderFunction { const FirstOrderFunctor& functor() const { return *functor_; } private: - // Tags used to differentiate between dynamic and fixed size constructor - // delegate invocations. - static constexpr std::integral_constant DYNAMIC_INIT{}; - static constexpr std::integral_constant FIXED_INIT{}; - - template - explicit NumericDiffFirstOrderFunction( - std::unique_ptr functor, - int num_parameters, - Ownership ownership, - const NumericDiffOptions& options, - InitTag /*unused*/) + explicit NumericDiffFirstOrderFunction(std::unique_ptr 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 dca67bacf..18900efe5 100644 --- a/internal/ceres/autodiff_cost_function_test.cc +++ b/internal/ceres/autodiff_cost_function_test.cc @@ -92,6 +92,30 @@ TEST(AutodiffCostFunction, BilinearDifferentiationTest) { delete cost_function; } +TEST(AutodiffCostFunction, OwnershipTest) { + BinaryScalarCost functor(1.0); + { + AutoDiffCostFunction 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>( + std::make_unique(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 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 5f1eb0b10..a75f0e7d2 100644 --- a/internal/ceres/autodiff_first_order_function_test.cc +++ b/internal/ceres/autodiff_first_order_function_test.cc @@ -73,5 +73,20 @@ TEST(AutoDiffFirstOrderFunction, BilinearDifferentiationTest) { EXPECT_EQ(gradient[3], parameters[2]); } +TEST(AutoDiffFirstOrderFunction, OwnershipTest) { + QuadraticCostFunctor functor(1.0); + { + AutoDiffFirstOrderFunction 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 5b7261ce0..019eb7905 100644 --- a/internal/ceres/dynamic_autodiff_cost_function_test.cc +++ b/internal/ceres/dynamic_autodiff_cost_function_test.cc @@ -820,4 +820,21 @@ TEST(DynamicAutoDiffCostFunctionTest, UniquePtr) { (void)DynamicAutoDiffCostFunction(std::make_unique()); } +TEST(DynamicAutoDiffCostFunctionTest, Ownership) { + MyCostFunctor functor; + { + DynamicAutoDiffCostFunction cost_function( + &functor, DO_NOT_TAKE_OWNERSHIP); + cost_function.AddParameterBlock(3); + cost_function.SetNumResiduals(3); + } +} + +TEST(DynamicAutoDiffCostFunctionTest, ExplicitUniquePtr) { + auto functor = std::make_unique(); + DynamicAutoDiffCostFunction 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 aec78197c..b4d7b73aa 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 @@ TEST(DynamicNumericdiffCostFunctionTest, ArgumentForwarding) { (void)DynamicNumericDiffCostFunction(); } -TEST(DynamicAutoDiffCostFunctionTest, UniquePtr) { +TEST(DynamicNumericDiffCostFunctionTest, UniquePtr) { (void)DynamicNumericDiffCostFunction( std::make_unique()); } +TEST(DynamicNumericDiffCostFunctionTest, Ownership) { + MyCostFunctor functor; + { + DynamicNumericDiffCostFunction 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 235c26618..0b393d766 100644 --- a/internal/ceres/numeric_diff_cost_function_test.cc +++ b/internal/ceres/numeric_diff_cost_function_test.cc @@ -460,4 +460,16 @@ TEST(NumericDiffCostFunction, UniquePtrCtor) { NumericDiffCostFunction>(); } +TEST(NumericDiffCostFunction, Ownership) { + EasyFunctor functor; + { + NumericDiffCostFunction 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 dc02b1350..bf5a39447 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 @@ TEST(NumericDiffFirstOrderFunction, BilinearDifferentiationTestDynamic) { EXPECT_NEAR(gradient[3], parameters[2], kTolerance); } +TEST(NumericDiffFirstOrderFunction, OwnershipTest) { + QuadraticCostFunctor functor(1.0); + { + NumericDiffFirstOrderFunction 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