changes made to build without warnings #143

aditya-vk · 2017-03-24T08:04:00Z

typedef (unused) -> set to ignore this warning. Need a fix.

Major warnings:
different types compared -> type casted as appropriate
incorrectly ordered initialization -> reordered
unused variables -> specified to have been unused

This change is

mkoval

Thanks for being diligent to fix these warnings. Since this is a rather pedantic pull request, I'll take this opportunity to be overly pedantic myself! 😉

Most of the casts to unsigned int probably should be casts to size_t instead.
Use static_cast instead of a C style casts.
Use AIKIDO_UPPER_CASE for macros to differentiate them from normal functions and variables.

Also, note that the hauser_parabolic_smoother subdirectory is imported from another project and doesn't use Aikido's coding standard. I recommend disabling all of the warnings in this subdirectory instead of trying to fix them - that just makes the diff with the tarball we imported unnecessarily larger.

mkoval · 2017-03-31T02:16:40Z

src/constraint/uniform/RnBoxConstraint.cpp

@@ -62,7 +62,7 @@ bool RnBoxConstraintSampleGenerator::sample(
 {
  Eigen::VectorXd value(mDistributions.size());

-  for (size_t i = 0; i < value.size(); ++i)
+  for (int i = 0; i < value.size(); ++i) // i should be the same type as size()


I don't think this comment is necessary.

aditya-vk · 2017-03-31T19:32:08Z

src/trajectory/Interpolated.cpp

@@ -90,11 +90,11 @@ void Interpolated::evaluate(double _t, State *_state) const
 void Interpolated::evaluateDerivative(double _t, int _derivative,
  Eigen::VectorXd& _tangentVector ) const
 {
-  if (_derivative == 0)
+  if (_derivative <= 0) // Invalid


I didn't realize I pushed this edit as well. This has nothing to do with warnings. I am changing it back to the original for now.

aditya-vk · 2017-03-31T21:36:18Z

static_cast of variables and unsigned int to size_t - check
ignore external project for warnings - check

mkoval · 2017-04-02T21:31:14Z

CMakeLists.txt

@@ -137,7 +137,7 @@ include_directories(${DART_INCLUDE_DIRS})
 # Libraries and unit tests.
 #
 # TODO: Add this to each target's DEFINITIONS property.
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Werror -Wno-unused-local-typedefs")


Why is -Wno-unused-local-typedefs necessary? Could we remove the unused typedefs instead? 😉

mkoval · 2017-04-02T21:32:40Z

src/statespace/dart/SE3Joint.cpp

-  : JointStateSpace(_joint)
-  , SE3()
+  : SE3()
+  , JointStateSpace(_joint) // Order of class members to be maintained


This comment is misleading. SE3 and JointSubSpace are super classes, not class members.

Ah. My bad. I'll fix it.

jslee02 · 2017-04-03T19:20:40Z

Can we merge this after #114 is merged? I'd like to make -Werror optional (default is off) and turn it on for CI tests.

mkoval · 2017-04-04T01:47:01Z

I approved #114. Let's merge that first, make the last few changes here, and merge this as well.

aditya-vk · 2017-04-04T03:23:46Z

I'll review this again first thing in the morning, address the suggestions by @mkoval. Should be ready for merge by morning.

aditya-vk · 2017-04-04T16:03:59Z

Should be ready for merge now. 😅

Removed unnecessary (and um erroneous) comments which weren't required
Removed the unused typedef and corresponding flag for compiling

jslee02 · 2017-04-04T16:18:43Z

Let me resolve the conflicts with master branch.

Conflicts: CMakeLists.txt src/external/hauser_parabolic_smoother/CMakeLists.txt

jslee02

The conflicts with master branch are resolved, but I still see warnings from the unit tests. @aditya-vk Could you resolve them by building Aikido with tests target as make -j4 tests?

jslee02 · 2017-04-04T16:44:31Z

src/statespace/dart/SE2Joint.cpp

-  , SE2()
+  : SE2()
+  , JointStateSpace(_joint)
+


Nit: Remove unnecessary empty line

aditya-vk · 2017-04-06T15:01:32Z

Builds without warnings even on unit tests.
ToDo: Run the same with -Wpedantic for a stricter check.

aditya-vk · 2017-04-07T03:16:42Z

I am not sure what to do with the following warning:
error: anonymous variadic macros were introduced in C99 [-Werror=variadic-macros]

Rest of the warnings have all been fixed with -Wpedantic as well.

mkoval

I believe you can fix the variadic-macros warning by adding SYSTEM to this line:

include_directories("${gtest_SOURCE_DIR}/include")

The only variadic macro I see in the project is in gtest, which should be flagged as a SYSTEM header so it doesn't generate warnings.

mkoval · 2017-04-07T03:19:13Z

src/external/hauser_parabolic_smoother/CMakeLists.txt

@@ -11,4 +11,5 @@ set_target_properties("${PROJECT_NAME}_external_hauserparabolicsmoother"
 )
 target_compile_options("${PROJECT_NAME}_external_hauserparabolicsmoother"
  PUBLIC ${AIKIDO_CXX_STANDARD_FLAGS}
+  PRIVATE -w


What is this? 😱

If I'm correct, this is to ignore warnings from ${PROJECT_NAME}_external_hauserparabolicsmoother target. Since it's not an external target, we cannot use include_directories() with SYSTEM option.

mkoval · 2017-04-07T03:19:50Z

src/trajectory/Interpolated.cpp

@@ -90,11 +90,11 @@ void Interpolated::evaluate(double _t, State *_state) const
 void Interpolated::evaluateDerivative(double _t, int _derivative,
  Eigen::VectorXd& _tangentVector ) const
 {
-  if (_derivative == 0)
+  if (_derivative == 0) // Invalid


Nit: Remove comment.

mkoval · 2017-04-07T03:20:57Z

tests/constraint/test_SO2UniformSampler.cpp

@@ -26,7 +26,7 @@ class SO2UniformSamplerTests : public ::testing::Test
    mRng = make_unique<RNGWrapper<std::default_random_engine>>(0);

    mTargets.clear();
-    for (int i = 0; i < NUM_TARGETS; ++i)
+    for (auto i = 0u; i < NUM_TARGETS; ++i)


Nit: Could we just make this a size_t? I'm generally a fan of almost always auto, but it's a bit funny that we changed a bunch of other loop variables to size_ in this pull request. 😉

aditya-vk · 2017-04-09T00:35:18Z

Woohoo! Done! I guess? :D

Added the SYSTEM flag in the CMakeLists.txt one level lower. I am not sure why including the flag in the tests/CMakeLists did not solve the issue..shouldn't it have?

jslee02

I am not sure why including the flag in the tests/CMakeLists did not solve the issue..shouldn't it have?

I think it's because of the location of the SYSTEM option. It should appear before directory.

Sorry for this PR keeps bothering you but I still see errors. 😓

Some warnings are compiler version dependent. If you cannot reproduce the error (warning), I can help to fix them.

Also, as the warnings from gtest is resolved, we might don't want to change the gtest code. It would be good to undo the changes you've made in the previous commits.

jslee02 · 2017-04-09T02:06:55Z

tests/CMakeLists.txt

@@ -1,5 +1,6 @@
 # GTest setup.
 add_subdirectory(gtest)
+
 include_directories("${gtest_SOURCE_DIR}/include")


Nit: This line is not added by this PR, but it seems redundant. "${gtest_SOURCE_DIR}/include" is included in the CMakeLists.txt under gtest subdirectory.

jslee02 · 2017-04-09T02:10:15Z

tests/constraint/test_TSR.cpp

@@ -18,15 +18,15 @@ using dart::common::make_unique;
 using DefaultRNG = RNGWrapper<std::default_random_engine>;


-static Eigen::Vector6d getPose(Eigen::Isometry3d isometry)
+static inline Eigen::Vector6d getPose(Eigen::Isometry3d isometry)


Does this really cause warnings? This function is defined in source code so we shouldn't worry about duplicated definitions. Am I missing something here?

From the documentation:
-Wunused-function
Warn whenever a static function is declared but not defined or a non-inline static function is unused. This warning is enabled by -Wall.
In our case a non-inline static function's not used.

It seems the real problem is that this function is unused. I think we could simply remove this function since it was defined only for the local tests but not used.

jslee02 · 2017-04-09T02:11:30Z

cmake/CompilerSettings.cmake

@@ -17,6 +17,8 @@ if(CMAKE_COMPILER_IS_GNUCXX)

  set(AIKIDO_CXX_STANDARD_FLAGS -std=c++11)

+  add_compile_options(-Wall)
+  add_compile_options(-Wpedantic)


Nit: We could put multiple options in one function like add_compile_options(-Wall -Wpedantic).

jslee02 · 2017-04-10T18:19:16Z

tests/constraint/test_NonColliding.cpp

@@ -35,17 +35,11 @@ class NonCollidingTest : public ::testing::Test
    auto boxNode = mBox->createJointAndBodyNodePair<FreeJoint>().second;
    Eigen::Vector3d boxSize(0.5, 0.5, 0.5);
    std::shared_ptr<BoxShape> boxShape(new BoxShape(boxSize));
-    auto shapeNode = boxNode->createShapeNodeWith
-                      <VisualAspect, CollisionAspect, DynamicsAspect>(boxShape);


You shouldn't remove the whole line, but just shapeNode variable. createShapeNodeWith<...>() creates ShapeNode within boxNode and returns the pointer. That means this function changes the internal state of bodyNode, which is necessary for this test.

jslee02 · 2017-04-10T18:19:45Z

tests/constraint/test_NonColliding.cpp

-    // Create a shpae node for visualization and collision checking
-    auto shapeNode1 = bn1->createShapeNodeWith
-                      <VisualAspect, CollisionAspect, DynamicsAspect>(box);
-


Please don't remove the whole line but shapeNode1. (see above)

coveralls · 2017-04-10T22:37:55Z

Coverage remained the same at 85.208% when pulling 21fe382 on werror_clean into a9b50ae on master.

jslee02 · 2017-04-10T22:41:44Z

All the warnings of more tighter warning levels are gone! 🎉

jslee02 · 2017-04-10T23:49:57Z

@mkoval Could you do take a look once more for sure?

changes made to build without warnings

926d505

aditya-vk assigned ClintLiddick and Shushman Mar 24, 2017

mkoval requested changes Mar 31, 2017

View reviewed changes

aditya-vk commented Mar 31, 2017

View reviewed changes

aditya-vk added 2 commits March 31, 2017 16:36

pr suggestions addressed. need to ignore external for warnings

a41a598

addressed the suggestions on PR

9b3a985

mkoval approved these changes Apr 2, 2017

View reviewed changes

jslee02 added this to the Aikido 0.1.0 milestone Apr 4, 2017

jslee02 self-requested a review April 4, 2017 03:37

removed unused typedef. removed unnecessary comments

823be29

jslee02 added 2 commits April 4, 2017 12:32

Merge remote-tracking branch 'origin/master' into werror_clean

a8db4b5

Conflicts: CMakeLists.txt src/external/hauser_parabolic_smoother/CMakeLists.txt

Suppress warning in PolynomialConstraint.cpp

e859eb9

jslee02 suggested changes Apr 4, 2017

View reviewed changes

src/statespace/dart/SE2Joint.cpp Outdated

, SE2()

: SE2()

, JointStateSpace(_joint)

Copy link

Member

jslee02 Apr 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Remove unnecessary empty line

jslee02 and others added 3 commits April 5, 2017 15:41

Resolve some of warnings

d6884b4

Resolve some of warnings in tests

6ba97bc

fix warnings with -Wall -Werror; tests included

8779ee9

aditya-vk added 5 commits April 6, 2017 14:41

nitpick

9b621e4

fix warnings

91cb9a5

fix warnings ompl

1db0113

minor fixes with pedantic. tests to be fixed still

62395e7

fixed warning with pedantic for tests as well

bffe6ec

mkoval requested changes Apr 7, 2017

View reviewed changes

jslee02 and others added 4 commits April 7, 2017 01:13

Merge branch 'master' into werror_clean

54d8a0d

nitpicks

6366a4b

added SYSTEM flag for Google Test .h files

69f689b

added SYSTEM flag to Google Test .h files

ecce168

jslee02 suggested changes Apr 9, 2017

View reviewed changes

Suppress warning from RnConstantSampler.cpp

5180540

jslee02 reviewed Apr 10, 2017

View reviewed changes

jslee02 added 10 commits April 10, 2017 14:31

Enable -Wextra and fixes the entailed warnings

e5f0d0f

Add back accidentally removed lines in test_NonColliding.cpp

d507361

Enable -Wextra and fixes the entailed warnings

16c7778

Rollback changes made on gtest

b132bd0

Remove unused static function from test_TSR.cpp

0eefb20

Set the warning flags for clang as well

8f2330f

Include gtest include directory to gtest target

2298066

Remove unintended empty lines in test_NonColliding.cpp

b92d42a

Remove duplicates of warning flags

57c5572

Address more warnings in rviz and planner/ompl

21fe382

jslee02 approved these changes Apr 10, 2017

View reviewed changes

mkoval approved these changes Apr 11, 2017

View reviewed changes

jslee02 merged commit a59975f into master Apr 11, 2017

jslee02 deleted the werror_clean branch April 11, 2017 01:35

jslee02 mentioned this pull request Apr 27, 2017

Consider enabling -Wall and -Wextra #115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes made to build without warnings #143

changes made to build without warnings #143

aditya-vk commented Mar 24, 2017 •

edited by mkoval

Loading

mkoval left a comment

mkoval Mar 31, 2017

aditya-vk Mar 31, 2017 •

edited

Loading

aditya-vk commented Mar 31, 2017 •

edited

Loading

mkoval Apr 2, 2017

mkoval Apr 2, 2017

aditya-vk Apr 4, 2017

jslee02 commented Apr 3, 2017

mkoval commented Apr 4, 2017

aditya-vk commented Apr 4, 2017 •

edited

Loading

aditya-vk commented Apr 4, 2017

jslee02 commented Apr 4, 2017 •

edited

Loading

jslee02 left a comment

jslee02 Apr 4, 2017

aditya-vk commented Apr 6, 2017

aditya-vk commented Apr 7, 2017

mkoval left a comment

mkoval Apr 7, 2017

jslee02 Apr 7, 2017

mkoval Apr 7, 2017

mkoval Apr 7, 2017

aditya-vk commented Apr 9, 2017 •

edited

Loading

jslee02 left a comment

jslee02 Apr 9, 2017

jslee02 Apr 9, 2017

aditya-vk Apr 10, 2017

jslee02 Apr 10, 2017 •

edited

Loading

jslee02 Apr 9, 2017

jslee02 Apr 10, 2017 •

edited

Loading

jslee02 Apr 10, 2017

coveralls commented Apr 10, 2017

jslee02 commented Apr 10, 2017 •

edited

Loading

jslee02 commented Apr 10, 2017

changes made to build without warnings #143

changes made to build without warnings #143

Conversation

aditya-vk commented Mar 24, 2017 • edited by mkoval Loading

mkoval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditya-vk Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

aditya-vk commented Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 commented Apr 3, 2017

mkoval commented Apr 4, 2017

aditya-vk commented Apr 4, 2017 • edited Loading

aditya-vk commented Apr 4, 2017

jslee02 commented Apr 4, 2017 • edited Loading

jslee02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditya-vk commented Apr 6, 2017

aditya-vk commented Apr 7, 2017

mkoval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditya-vk commented Apr 9, 2017 • edited Loading

jslee02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 10, 2017

jslee02 commented Apr 10, 2017 • edited Loading

jslee02 commented Apr 10, 2017

aditya-vk commented Mar 24, 2017 •

edited by mkoval

Loading

aditya-vk Mar 31, 2017 •

edited

Loading

aditya-vk commented Mar 31, 2017 •

edited

Loading

aditya-vk commented Apr 4, 2017 •

edited

Loading

jslee02 commented Apr 4, 2017 •

edited

Loading

aditya-vk commented Apr 9, 2017 •

edited

Loading

jslee02 Apr 10, 2017 •

edited

Loading

jslee02 Apr 10, 2017 •

edited

Loading

jslee02 commented Apr 10, 2017 •

edited

Loading