C++20, compiler warnings#5
Conversation
wyattrees
force-pushed
the
compiler-warnings
branch
from
e8891be to
dd784d7
Compare
| : frame_(Frame::identity()) | ||
| , rotation_scale_(0.5) |
There was a problem hiding this comment.
If you wanted to take the modernization even further, you could look for data members like this which are initialized with constants and move their initialization to where they're declared.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-default
This can easily be saved for a later PR, but I wanted to mention it since it's one of my fav C++ features.
| enable_doxygen() | ||
|
|
||
| # allow for static analysis options | ||
| include(cmake/StaticAnalyzers.cmake) |
There was a problem hiding this comment.
Unless CI is also exercising the sanitizers, doxygen, and static analyzers, I'm not convinced this is worth adding.
| BioIKKinematicsPlugin() { enable_profiler = false; } | ||
|
|
||
| virtual const std::vector<std::string> &getJointNames() const { | ||
| virtual const std::vector<std::string> &getJointNames() const override { |
There was a problem hiding this comment.
override implies virtual so if you're using override you can remove virtual.
| std::string *error_text_out = 0) const { | ||
| virtual bool supportsGroup( | ||
| const moveit::core::JointModelGroup */*jmg*/, | ||
| std::string */*error_text_out*/ = 0) const override { |
There was a problem hiding this comment.
You changed the type of these parameters. They used to be pointer types and now they're values.
There was a problem hiding this comment.
But there's still a '*' before the commented out parameter names?
| BASE* create(ARGS... args) const { return new DERIVED(args...); } | ||
| BASE* clone(const BASE* o) const { return new DERIVED(*(const DERIVED*)o); } | ||
| Class(const std::string& name) | ||
| BASE* clone(const BASE* o) const { return new DERIVED(*dynamic_cast<const DERIVED*>(o)); } |
There was a problem hiding this comment.
If this dynamic cast fails, a null pointer will be dereferenced. How sure are we that it will never fail?
There was a problem hiding this comment.
Not really sure, to be honest. I think if we verify that all of the different kinematic solver modes work, as this is used to instantiate the different types of solvers, then it should be OK. I'll run a quick test to be sure.
|
@tylerjw If you think bio_ik deserves some more attention, check out this PR. It makes some much needed best practices improvements. |
wyattrees
force-pushed
the
compiler-warnings
branch
from
dd784d7 to
680b5c5
Compare
| { | ||
| Vector3 pos; | ||
| double __padding[4 - (sizeof(Vector3) / sizeof(double))]; | ||
| // TODO(wyattrees): is __padding needed? rename without double underscore? |
There was a problem hiding this comment.
This seems like it might be for getting the Quaternion to be on some address multiple so it is optimized for simd instructions. Honestly I feel like much of these optimizations here are not justified without benchmarks and make the code considerably harder to read without any justification for their existance.
wyattrees
force-pushed
the
compiler-warnings
branch
from
680b5c5 to
1bb5899
Compare
|
note that c++20 has been dropped in favor of c++17 for Humble support |
wyattrees
force-pushed
the
compiler-warnings
branch
from
1bb5899 to
57c30ca
Compare
3 participants
For a list of compiler warnings used, see
cmake/CompilerWarnings.cmake.This PR omits running
clang-format, as the diff would be nearly impossible to tell what actually meaningful changes were made.