andyz/squashed_improvements branch, merged w/master, `clang` tests fixed by zultron · Pull Request #63 · PickNikRobotics/trackjoint

zultron · 2020-09-17T18:41:56Z

Halfway down the rabbit hole fixing the failed CI checks in #62, I realized this needs a new PR, both to be more reviewable (these changes make a big diff) and so I can see new CI results while I work.

Changes here:

Merge with master branch to pull in @nbbrooks 's earlier cleanups
Re-run clang-format and clang-tidy
Pull my other clean-up commits from Zultron/tracksuite refactor pt1 #62 to this PR, where they fit better

- Remove unnecessary inline tags - Move remaining inline function implementations into header

…ovements_merged - Minor conflicts resolved - Additional `inline` specifiers removed following "Clean up inline functions (#54)" - New `kNoError` -> `NO_ERROR` conversions

Command copied from `moveit_ci`: find . -name '*.h' -or -name '*.hpp' -or -name '*.cpp' | \ xargs /usr/bin/clang-format -i -style=file

zultron · 2020-09-17T19:11:09Z

The Travis failures here are due to the failing NoisyStreamingCommand test. No more failures to to moveit_ci's clang tests.

zultron · 2020-09-17T19:15:46Z

Also, since this PR includes a merge with master, once @nbbrooks is done reviewing, it can be merged with no conflicts.

AndyZe · 2020-09-18T21:04:04Z

I see on CI that it's the NoisyStreamingCommand test that fails. That's to be expected at this point, won't prevent me from merging it.

AndyZe

Overall, this looks great. The only 2 things holding me back are:

enum naming
adding catkin_tools folder to .gitignore

AndyZe · 2020-09-18T21:05:54Z

.catkin_tools/README

@@ -1,13 +0,0 @@
-# Catkin Tools Metadata


Good idea to delete this stuff. Can you add it to .gitignore so it doesn't get committed accidentally again?

Fixed in b3cfcbf

AndyZe · 2020-09-18T21:11:02Z

include/trackjoint/error_codes.h

+  LIMIT_NOT_POSITIVE = 6,
+  GOAL_POSITION_MISMATCH = 7,
+  FAILURE_TO_GENERATE_SINGLE_WAYPOINT = 8,
+  LESS_THAN_TEN_TIMESTEPS_FOR_STREAMING_MODE = 9


I can't remember what style guide we're using for this project. The Google style guide says this should be kNoError:

https://google.github.io/styleguide/cppguide.html#Enumerator_Names

@nbbrooks is it cool if we specify Google style for this repo?

I think it makes sense to use Google style because we are using Google clang format

Related PR that should either be closed or merged: #60

I'm not arguing for a naming convention this way or that, but those changes were introduced in #53, and made their way into this PR with the merge from master branch. See here:

https://github.com/PickNikRobotics/trackjoint/pull/53/files#diff-c036aecfa9831ff50f1e589083907289

OK. We must be using the ROS style guide then. Would you mind reviewing/merging this PR so that will be documented in the README?

#60

ROS style guide: http://wiki.ros.org/CppStyleGuide

ugh, should have reviewed this a while ago

AndyZe · 2020-09-18T21:13:02Z

include/trackjoint/error_codes.h

-      { kLessThanTenTimestepsForStreamingMode, "In streaming mode, desired duration should be at least 10 "
-                                               "timesteps" } });
+const std::unordered_map<uint, std::string> ERROR_CODE_MAP({ { NO_ERROR, "No error, trajectory generation was "
+                                                                         "successful" },


whew, this autoformatting is atrocious. Oh well, it is what it is

I wouldn't ding your original formatting along stylistic lines, and compared to what clang-format did, yours is actually readable.

I think we should look for a clang-format option that makes this look better, or else disable the clang-format checks in CI so we can get passing builds without mangling our formatting.

Setting AlwaysBreakBeforeMultilineStrings: true in .clang-format stops this horrible behavior.

Fixed in e12fb23

PickNikRobotics/roscpp_code_format#14

@AndyZe

Fix hideous string breaks pointed out by @AndyZe

[...]/src/single_joint_generator.cpp: In member function ‘void trackjoint::SingleJointGenerator::extendTrajectoryDuration()’: [...]/src/single_joint_generator.cpp:123:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for (size_t idx = 0; idx < waypoints_.elapsed_times.size(); ++idx) ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [...]/src/single_joint_generator.cpp:138:19: warning: unused variable ‘error_code’ [-Wunused-variable] ErrorCodeEnum error_code = forwardLimitCompensation(&index_last_successful_); ^~~~~~~~~~

[...]/src/single_joint_generator.cpp: In member function ‘void trackjoint::SingleJointGenerator::extendTrajectoryDuration()’: [...]/src/single_joint_generator.cpp:123:30: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] for (size_t idx = 0; idx < waypoints_.elapsed_times.size(); ++idx) ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[...]/test/trajectory_generation_test.cpp: In member function ‘virtual void trackjoint::TrajectoryGenerationTest_VelAccelJerkLimit_Test::TestBody()’: [...]/test/trajectory_generation_test.cpp:433:16: error: unused variable ‘duration_tolerance’ [-Werror=unused-variable] const double duration_tolerance = 5e-3; ^~~~~~~~~~~~~~~~~~

@AndyZe

These files were removed in previous "Remove junk files" commit; @AndyZe suggested we keep them out permanently by `.gitignore`ing them.

Remove unneeded `static_cast<int>`; prepare for upcoming changes.

[...]/src/trajectory_generator.cpp:32:58: warning: redundant boolean literal in ternary expression result [readability-simplify-boolean-expr] bool timestep_was_upsampled = (upsample_rounds_ > 0) ? true : false; ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~ upsample_rounds_ > 0 [...]/src/trajectory_generator.cpp:61:58: warning: redundant boolean literal in ternary expression result [readability-simplify-boolean-expr] bool timestep_was_upsampled = (upsample_rounds_ > 0) ? true : false; ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~ upsample_rounds_ > 0

[...]/src/single_joint_generator.cpp:116:14: warning: local copy 'spline' of the variable 'fit' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization] Spline1D spline(fit); ~~~~~~~~ ^ const &

[...]/src/single_joint_generator.cpp:279:5: warning: Value stored to 'delta_v' is never read [clang-analyzer-deadcode.DeadStores] delta_v = 0; ^ [...]/src/single_joint_generator.cpp:279:5: note: Value stored to 'delta_v' is never read 1 warning generated.

/usr/include/clang/6.0.0/include/emmintrin.h:1593:10: warning: Dereference of null pointer [clang-analyzer-core.NullDereference] return *(__m128d*)__dp; ^ [...]/test/trajectory_generation_test.cpp:1093:33: note: Calling 'TrajectoryGenerationTest::calculatePositionAccuracy' const double position_error = calculatePositionAccuracy(goal_joint_states_, output_trajectories_); ^ [...]/test/trajectory_generation_test.cpp:146:28: note: Assuming the condition is false for (size_t joint = 0; joint < trajectory.size(); ++joint) ^ [...]/test/trajectory_generation_test.cpp:146:5: note: Loop condition is false. Execution continues on line 152 for (size_t joint = 0; joint < trajectory.size(); ++joint) ^ [...]/test/trajectory_generation_test.cpp:152:20: note: Calling 'MatrixBase::norm' double error = (final_positions - goal_positions).norm(); ^ /usr/include/eigen3/Eigen/src/Core/Dot.h:107:23: note: Calling 'MatrixBase::squaredNorm' return numext::sqrt(squaredNorm()); ^ /usr/include/eigen3/Eigen/src/Core/Dot.h:95:23: note: Calling 'DenseBase::sum' return numext::real((*this).cwiseAbs2().sum()); ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:451:6: note: Left side of '||' is false if(SizeAtCompileTime==0 || (SizeAtCompileTime==Dynamic && size()==0)) ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:451:31: note: Left side of '&&' is true if(SizeAtCompileTime==0 || (SizeAtCompileTime==Dynamic && size()==0)) ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:451:3: note: Taking false branch if(SizeAtCompileTime==0 || (SizeAtCompileTime==Dynamic && size()==0)) ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:453:10: note: Calling 'DenseBase::redux' return derived().redux(Eigen::internal::scalar_sum_op<Scalar,Scalar>()); ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:418:10: note: Calling 'redux_impl::run' return internal::redux_impl<Func, ThisEvaluator>::run(thisEval, func); ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:231:8: note: Assuming 'alignedSize' is not equal to 0 if(alignedSize) ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:231:5: note: Taking true branch if(alignedSize) ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:233:34: note: Calling 'redux_evaluator::packet' PacketScalar packet_res0 = mat.template packet<alignment,PacketScalar>(alignedStart); ^ /usr/include/eigen3/Eigen/src/Core/Redux.h:377:12: note: Calling 'unary_evaluator::packet' { return m_evaluator.template packet<LoadMode,PacketType>(index); } ^ /usr/include/eigen3/Eigen/src/Core/CoreEvaluators.h:563:31: note: Calling 'binary_evaluator::packet' return m_functor.packetOp(m_argImpl.template packet<LoadMode, PacketType>(index)); ^ /usr/include/eigen3/Eigen/src/Core/CoreEvaluators.h:734:31: note: Calling 'evaluator::packet' return m_functor.packetOp(m_lhsImpl.template packet<LoadMode,PacketType>(index), ^ /usr/include/eigen3/Eigen/src/Core/CoreEvaluators.h:204:41: note: Passing null pointer value via 1st parameter 'from' return ploadt<PacketType, LoadMode>(m_data + index); ^ /usr/include/eigen3/Eigen/src/Core/CoreEvaluators.h:204:12: note: Calling 'ploadt' return ploadt<PacketType, LoadMode>(m_data + index); ^ /usr/include/eigen3/Eigen/src/Core/GenericPacketMath.h:462:3: note: Taking true branch if(Alignment >= unpacket_traits<Packet>::alignment) ^ /usr/include/eigen3/Eigen/src/Core/GenericPacketMath.h:463:26: note: Passing null pointer value via 1st parameter 'from' return pload<Packet>(from); ^ /usr/include/eigen3/Eigen/src/Core/GenericPacketMath.h:463:12: note: Calling 'pload' return pload<Packet>(from); ^ /usr/include/eigen3/Eigen/src/Core/arch/SSE/PacketMath.h:307:124: note: Passing null pointer value via 1st parameter '__dp' template<> EIGEN_STRONG_INLINE Packet2d pload<Packet2d>(const double* from) { EIGEN_DEBUG_ALIGNED_LOAD return _mm_load_pd(from); } ^ /usr/include/eigen3/Eigen/src/Core/arch/SSE/PacketMath.h:307:112: note: Calling '_mm_load_pd' template<> EIGEN_STRONG_INLINE Packet2d pload<Packet2d>(const double* from) { EIGEN_DEBUG_ALIGNED_LOAD return _mm_load_pd(from); } ^ /usr/include/clang/6.0.0/include/emmintrin.h:1593:10: note: Dereference of null pointer return *(__m128d*)__dp; ^

AndyZe · 2020-09-20T21:55:36Z

Looks good to me! I think we can re-run clang format when that gets sorted out.

nbbrooks and others added 5 commits May 18, 2020 09:05

Clang tidy fixes (#53)

518206d

Clean up inline functions (#54)

29b975b

- Remove unnecessary inline tags - Move remaining inline function implementations into header

Update executable names in Readme (#56)

250aa4b

Merge remote-tracking branch 'origin/master' into andyz/squashed_impr…

91fdaf7

…ovements_merged - Minor conflicts resolved - Additional `inline` specifiers removed following "Clean up inline functions (#54)" - New `kNoError` -> `NO_ERROR` conversions

Run clang-format

962197d

Command copied from `moveit_ci`: find . -name '*.h' -or -name '*.hpp' -or -name '*.cpp' | \ xargs /usr/bin/clang-format -i -style=file

AndyZe mentioned this pull request Sep 17, 2020

Squashed improvements #58

Closed

zultron mentioned this pull request Sep 17, 2020

Zultron/tracksuite refactor pt1 #62

Merged

zultron changed the title ~~Zultron/andyz/squashed improvements merged~~ andyz/squashed_improvements branch, merged w/master, clang tests fixed Sep 17, 2020

AndyZe suggested changes Sep 18, 2020

View reviewed changes

zultron added 11 commits September 20, 2020 01:46

Adjust clang-format rules and re-run

e12fb23

Fix hideous string breaks pointed out by @AndyZe

Remove junk files

9bbf67d

.gitignore generated .catkin-tools directory

b3cfcbf

These files were removed in previous "Remove junk files" commit; @AndyZe suggested we keep them out permanently by `.gitignore`ing them.

Change int to size_t for vector indices in tests

f48cf32

Remove unneeded `static_cast<int>`; prepare for upcoming changes.

single_joint_generator.cpp: Fix clang-tidy errors

f9fd220

[...]/src/single_joint_generator.cpp:116:14: warning: local copy 'spline' of the variable 'fit' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization] Spline1D spline(fit); ~~~~~~~~ ^ const &

Slence clang-tidy warnings

58fc12a

[...]/src/single_joint_generator.cpp:279:5: warning: Value stored to 'delta_v' is never read [clang-analyzer-deadcode.DeadStores] delta_v = 0; ^ [...]/src/single_joint_generator.cpp:279:5: note: Value stored to 'delta_v' is never read 1 warning generated.

zultron force-pushed the zultron/andyz/squashed_improvements_merged branch from 5c964cc to a40551f Compare September 20, 2020 06:48

zultron mentioned this pull request Sep 20, 2020

Update Readme that we are not using Google style #60

Closed

AndyZe approved these changes Sep 20, 2020

View reviewed changes

AndyZe merged commit b006cbb into PickNikRobotics:andyz/squashed_improvements Sep 20, 2020

zultron deleted the zultron/andyz/squashed_improvements_merged branch September 21, 2020 02:27

Conversation

zultron commented Sep 17, 2020

Uh oh!

zultron commented Sep 17, 2020

Uh oh!

zultron commented Sep 17, 2020

Uh oh!

AndyZe commented Sep 18, 2020

Uh oh!

AndyZe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyZe commented Sep 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants