Skip to content

Commit

Permalink
Problem.hh: allow the use of shared_ptr for the constructor
Browse files Browse the repository at this point in the history
We kept the old API for backwards compatibility (for now...).
  • Loading branch information
Benjamin Chrétien committed Sep 17, 2015
1 parent 9508b31 commit 6cafd4c
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 5 deletions.
27 changes: 25 additions & 2 deletions include/roboptim/core/problem.hh
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,17 @@ namespace roboptim
/// \{

/// \pre costfunction \f$\mathbb{R}^n \rightarrow \mathbb{R}\f$

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Sep 17, 2015

Member

Remove duplicated comment?

/// \brief Deprecated constructor taking a reference to a cost function.
/// This legacy version meant that we simply kept a const reference to the
/// cost function, which could reference stack variables...
/// This prepares the transition to something safer (shared_ptr).
/// \param cost cost function.
explicit Problem (const function_t& cost);
explicit Problem (const function_t& cost) ROBOPTIM_CORE_DEPRECATED;

/// \pre costfunction \f$\mathbb{R}^n \rightarrow \mathbb{R}\f$
/// \brief Constructor taking a shared_ptr to a cost function.
/// \param cost cost function.
explicit Problem (const boost::shared_ptr<function_t>& cost);

/// \brief Copy constructor.
/// \param pb problem to copy.
Expand Down Expand Up @@ -285,11 +294,25 @@ namespace roboptim
/// \return output stream
std::ostream& print (std::ostream& o) const;

private:
/// \brief Custom deleter that does not delete anything.
/// This can be used when creating a shared_ptr from a reference, although

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Sep 17, 2015

Member

/// To be removed ASAP ?

This comment has been minimized.

Copy link
@bchretien

bchretien Sep 17, 2015

Member

That's the objective, but I can add a comment. I was actually wondering: how many releases should be done before breaking backwards compatibility once a method has been flagged as deprecated?

/// this should be used with **great** care...
struct NoopDeleter
{
inline void operator() (function_t*) const {}
};

private:
/// \brief Objective function.
const function_t& function_;
/// Note: do not give access to this shared_ptr, since for now the legacy
/// API relying on a simple reference prevents any proper memory
/// management.
const boost::shared_ptr<function_t> function_;

/// \brief Starting point.
startingPoint_t startingPoint_;

/// \brief Vector of constraints.
constraints_t constraints_;

Expand Down
25 changes: 23 additions & 2 deletions include/roboptim/core/problem.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@ namespace roboptim
//
template <typename T>

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Sep 17, 2015

Member

// TODO: remove?

This comment has been minimized.

Copy link
@bchretien

bchretien Sep 17, 2015

Member

This is implied by the ROBOPTIM_CORE_DEPRECATED flag + the doc associated with it, so I don't think a comment is required right here.

Problem<T>::Problem (const function_t& f)
: function_ (f),
// Note: kids, don't do this at home! Until now, we kept a reference to
// the function passed, which is just as bad. This prepares the transition
// to the safer shared_ptr version.
: function_ (const_cast<function_t*> (&f), NoopDeleter ()),
startingPoint_ (),
constraints_ (),
boundsVect_ (),
Expand All @@ -54,6 +57,24 @@ namespace roboptim
std::fill (argumentScaling_.begin (), argumentScaling_.end (), 1.);
}

template <typename T>
Problem<T>::Problem (const boost::shared_ptr<function_t>& f)
: function_ (f),
startingPoint_ (),
constraints_ (),
boundsVect_ (),
argumentBounds_ (static_cast<std::size_t> (f->inputSize ())),
scalingVect_ (),
argumentScaling_ (static_cast<std::size_t> (f->inputSize ())),
argumentNames_ ()

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Sep 17, 2015

Member

assert(f.get() != 0);

This comment has been minimized.

Copy link
@bchretien

bchretien Sep 17, 2015

Member

👍

{
// Initialize bound.
std::fill (argumentBounds_.begin (), argumentBounds_.end (),
function_t::makeInfiniteInterval ());
// Initialize scaling.

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Sep 17, 2015

Member

Avoid duplication, introduce a method to initialize the attributes?

This comment has been minimized.

Copy link
@bchretien

bchretien Sep 17, 2015

Member

I was thinking about it. I'll add that with the rest.

std::fill (argumentScaling_.begin (), argumentScaling_.end (), 1.);
}

template <typename T>
Problem<T>::~Problem ()
{
Expand All @@ -77,7 +98,7 @@ namespace roboptim
const typename Problem<T>::function_t&
Problem<T>::function () const
{
return function_;
return *function_;
}

template <typename T>
Expand Down
9 changes: 8 additions & 1 deletion tests/problem.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ BOOST_AUTO_TEST_CASE_TEMPLATE (problem, T, functionTypes_t)
typename constantFunction_t::vector_t v (2);
v.setZero ();

constantFunction_t f (v);
// For shared_ptr constructor
boost::shared_ptr<constantFunction_t>
f = boost::make_shared<constantFunction_t> (v);

problem_t pb_fail (f);

Expand Down Expand Up @@ -114,6 +116,11 @@ BOOST_AUTO_TEST_CASE_TEMPLATE (problem, T, functionTypes_t)
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Sep 17, 2015

Member

Introduce a macro to hide the pragma? Define it in debug.hh or something similar?

This comment has been minimized.

Copy link
@bchretien

bchretien Sep 17, 2015

Member

Something like ROBOPTIM_ALLOW_DEPRECATION? With the push/pop behavior, I don't know what's the cleanest way. Here this was just to please GCC while building tests involving deprecated features before their removal.

This comment has been minimized.

Copy link
@bchretien

bchretien Sep 18, 2015

Member

Well, pragmas in macros are only possible with _Pragma(), which is available in C++11/C99 it seems (so 👍 for #73). It would look like:

# define ROBOPTIM_ALLOW_DEPRECATED_ON    \
  /* Disable deprecated warning */       \
  _Pragma ("GCC diagnostic push")        \
  _Pragma ("GCC diagnostic ignored \"-Wdeprecated-declarations\"")

# define ROBOPTIM_ALLOW_DEPRECATED_OFF \
  /* Re-enable deprecated warning */   \
  _Pragma ("GCC diagnostic pop")

Then:

ROBOPTIM_ALLOW_DEPRECATED_ON;
// Do deprecated stuff
// ...
ROBOPTIM_ALLOW_DEPRECATED_OFF;

gcc/clang seem to be accepting that without any complaint on Arch. I don't know if the same thing can be said for older versions (e.g. the ones on Ubuntu 12.04).

BOOST_CHECK (pb.argumentScales () == pb.argumentScaling ());
BOOST_CHECK (pb.scalesVector () == pb.scalingVector ());

// For legacy const ref constructor
constantFunction_t fRef (v);
problem_t pbRef (fRef);
(*output) << pbRef << std::endl;
#pragma GCC diagnostic pop

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Sep 17, 2015

Member

ditto


// Test a problem with multiple types of constraints.
Expand Down
26 changes: 26 additions & 0 deletions tests/problem.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,32 @@ Problem:
offset = [2](0,0)
Arguments bounds: (-inf, inf), (-inf, inf)
Arguments scaling: 1, 1
No constraints.
No starting point.
Infinity value (for all functions): inf
Problem:
Constant function
offset = [2](0,0)
Arguments bounds: (-inf, inf), (-inf, inf)
Arguments scaling: 1, 1
Arguments names: x₀, x₁
Number of constraints: 2
Constraint 0
Constant function
offset = [2](0,0)
Bounds: (-inf, inf), (-inf, inf)
Scaling: 1, 1
Initial value: [2](0, 0)
Constraint 1
Constant function
offset = [2](0,0)
Bounds: (-inf, inf), (-inf, inf)
Scaling: 1, 1
Initial value: [2](0, 0)
Starting point: [2](0,0)
Starting value: [2](0,0)
Infinity value (for all functions): inf

Arguments names: x₀, x₁
Number of constraints: 2
Constraint 0
Expand Down

0 comments on commit 6cafd4c

Please sign in to comment.