Introduce use of override keyword for virtual methods

[Guillaume227]

Added corresponding gcc warning to flag missing overrides.

Context: I am getting my feet wet with this library and I figured it would be a good modernization effort to clarify the internal interfaces with the introduction of the override keyword instead of indiscriminate virtual which is pre-c++11 fashion.

[andreadelprete]

Nice, did not know this keyword, but it makes a lot of sense as it clarifies the code!

[nim65s]

Thanks for helping !
But why are you removing those const keywords ?

[Guillaume227]

Well spotted, let me motivate that change.

Basic explanation:
In a method declaration (as opposed to a method definition) taking parameters by value, const are useless and are best omitted for clarity.
That's because we are passing a copy to the method anyway so const doesn't add anything and is not part of the method signature.

In other words it's ok (and good practice) to have:
in h file:
void foo(int arg);
in cpp file:

void foo(int const arg) {
 ...
 // the const in the declaration makes clear the local arg variable is not modified in the body of the function.
}

That will compile fine.

Let me stress the compiler knows it's the same foo function there, the const is only relevant in the local body context.
That's not the same story if you are passing the argument by reference.
E.g.:
in h file:
void foo(int& arg);
is not the same function as:
void foo(int const& arg);
and will result in a compilation error.

You might find that better explained there (doc for abseil, the google flagship C++ library):
https://abseil.io/tips/109

I took the opportunity to remove the const in the methods I was touching due to introducing override. There are more places that would benefit from a const clean-up but I did not want to mix concerns. I can follow-up with another PR later.

Benefits of this is that it's easier to tell when glancing at the header file when you are passing arguments by const reference or by value and generally means less verbose and cleaner interfaces.

[Guillaume227]

I see one problem with the pre-commit hook formatting, which introduces whitespace in macro arguments, leading to nasty warnings:

We would need to turn that check off for that kind of expressions - do you know how to do that?

[olivier-stasse]

I see one problem with the pre-commit hook formatting, which introduces whitespace in macro arguments,

According to https://clang.llvm.org/docs/ClangFormatStyleOptions.html
may be you could try to add:
WhitespaceSensitiveMacros: ['TSID_DISABLE_WARNING']
in the field args of

tsid/.pre-commit-config.yaml

Line 27 in 0a83575

args: ['--style={BasedOnStyle: Google,SortIncludes: false}']

[Guillaume227]

It seems to work nicely, thank you @olivier-stasse

[nim65s]

Thanks for your explanation. I was concerned this might be a breaking change if downstream code relied on this const, but it looks like this is not the case.

[nim65s]

I fixed the PR to target devel. But the history is messy now, do you want to clean it @Guillaume227, or should I handle this ?

Also, gitlab-CI is not happy on building the python bindings. Is it working for you ?

In file included from /usr/include/boost/preprocessor/iteration/detail/iter/forward1.hpp:48,
from /usr/include/boost/python/detail/invoke.hpp:62,
from /usr/include/boost/python/detail/caller.hpp:16,
from /usr/include/boost/python/object/function_handle.hpp:8,
from /usr/include/boost/python/converter/arg_to_python.hpp:19,
from /usr/include/boost/python/call.hpp:15,
from /usr/include/boost/python/object_core.hpp:14,
from /usr/include/boost/python/args.hpp:22,
from /usr/include/boost/python.hpp:11,
from /opt/openrobots/include/eigenpy/fwd.hpp:73,
from /opt/openrobots/include/eigenpy/eigenpy.hpp:9,
from /root/robotpkg/optimization/py-tsid/work/tsid-1.7.0/include/tsid/bindings/python/fwd.hpp:29,
from /root/robotpkg/optimization/py-tsid/work/tsid-1.7.0/include/tsid/bindings/python/solvers/solver-HQP-eiquadprog.hpp:21,
from /root/robotpkg/optimization/py-tsid/work/tsid-1.7.0/include/tsid/bindings/python/solvers/expose-solvers.hpp:21,
from /root/robotpkg/optimization/py-tsid/work/tsid-1.7.0/bindings/python/solvers/solver-HQP-eiquadprog.cpp:18:
/usr/include/boost/python/detail/invoke.hpp: In instantiation of 'PyObject* boost::python::detail::invoke(boost::python::detail::invoke_tag_<false, true>, const RC&, F&, TC&) [with RC = boost::python::detail::specify_a_return_value_policy_to_wrap_functions_returning<const tsid::solvers::QPDataQuadProgTpl<double>&>; F = const tsid::solvers::QPDataQuadProgTpl<double>& (tsid::solvers::SolverHQuadProgFast::*)() const; TC = boost::python::arg_from_python<tsid::solvers::SolverHQuadProgFast&>; PyObject = _object]':
/usr/include/boost/python/detail/caller.hpp:233:46:   required from 'PyObject* boost::python::detail::caller_arity<1>::impl<F, Policies, Sig>::operator()(PyObject*, PyObject*) [with F = const tsid::solvers::QPDataQuadProgTpl<double>& (tsid::solvers::SolverHQuadProgFast::*)() const; Policies = boost::python::default_call_policies; Sig = boost::mpl::vector2<const tsid::solvers::QPDataQuadProgTpl<double>&, tsid::solvers::SolverHQuadProgFast&>; PyObject = _object]'
/usr/include/boost/python/object/py_function.hpp:38:33:   required from 'PyObject* boost::python::objects::caller_py_function_impl<Caller>::operator()(PyObject*, PyObject*) [with Caller = boost::python::detail::caller<const tsid::solvers::QPDataQuadProgTpl<double>& (tsid::solvers::SolverHQuadProgFast::*)() const, boost::python::default_call_policies, boost::mpl::vector2<const tsid::solvers::QPDataQuadProgTpl<double>&, tsid::solvers::SolverHQuadProgFast&> >; PyObject = _object]'
/usr/include/boost/python/object/py_function.hpp:36:15:   required from here
/usr/include/boost/python/detail/invoke.hpp:86:14: error: no match for call to '(const boost::python::detail::specify_a_return_value_policy_to_wrap_functions_returning<const tsid::solvers::QPDataQuadProgTpl<double>&>) (const tsid::solvers::QPDataQuadProgTpl<double>&)'
86 |     return rc( (tc().*f)(BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, ac, () BOOST_PP_INTERCEPT)) );
|            ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Guillaume227]

Do we have a clean baseline for those checks? I originally looked into the clang-format warning issue because I thought it might be related to my change but it turns out it was a preexisting condition.

From the README it wasn't super clear to me how to build the python bindings so I have been skipping that part of the build locally for now. I ought to take another look at this. What build command do you use for the bindings?

[Guillaume227]

What do you mean by messy history by the way? The number of commits? Would a squash-merge address this without having to change the PR?

[nim65s]

About the history, I think this PR is only about b54b60f, and the 6 other commits shouldn't be here. Or maybe one more commit from pre-commit.ci bot.

I don't think a big squash would be the best option, especially because 0a83575 has a huge number of changes and was already merged.

ROS ci is currently broken, but gitlab-ci is green on the devel branch : https://gitlab.laas.fr/stack-of-tasks/tsid/-/pipelines/37298, so it should stay green here. (we can ignore the warning for the format job for now)

[Guillaume227]

Oh I totally see what you mean by messy now. Let me create a new PR.
How come #218 was merged in master and is not on devel by the way? Is that an oversight? Should master be merged into devel first? @nim65s: do you mind clarifying that?