Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce use of override keyword for virtual methods #224

Open
wants to merge 7 commits into
base: devel
Choose a base branch
from

Conversation

Guillaume227
Copy link
Contributor

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.

egordv and others added 3 commits February 2, 2024 09:28
* TaskTwoFramesEquality and ContactTwoFramePositions added with appropriate bindings. 

* getMotionTask type changed from TaskSE3Equality to TaskMotion to allow contacts use different motion tasks, not only TaskSE3Equality

* python demo of talos gripper with closed kinematic chain, using URDF/STLs in external repo


---------
Authored-by: @egordv
@andreadelprete
Copy link
Collaborator

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

@nim65s
Copy link
Contributor

nim65s commented Mar 14, 2024

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

@Guillaume227
Copy link
Contributor Author

Guillaume227 commented Mar 14, 2024

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
Copy link
Contributor Author

I see one problem with the pre-commit hook formatting, which introduces whitespace in macro arguments, leading to nasty warnings:
image
We would need to turn that check off for that kind of expressions - do you know how to do that?

@olivier-stasse
Copy link
Member

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

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

@Guillaume227
Copy link
Contributor Author

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

@nim65s
Copy link
Contributor

nim65s commented Mar 18, 2024

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 nim65s changed the base branch from master to devel March 18, 2024 14:05
@nim65s
Copy link
Contributor

nim65s commented Mar 18, 2024

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
Copy link
Contributor Author

Guillaume227 commented Mar 18, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

nim65s commented Mar 18, 2024

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
Copy link
Contributor Author

Guillaume227 commented Mar 18, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants