Fix compiler warnings and default to -Wall #53

MichaelEischer · 2016-02-11T18:58:36Z

This fixes all compiler warnings using g++ 5.2.1 (Ubuntu 15.10). Compiling should now also be possible using clang, but that results in a strange linker error for me (probably an incompatibility between g++ 5 and clang related to std::string).

Most changes are unsigned vs signed int, unused variables.

Special ones:

Allocate marker array in cmpattern_teamdetector.cpp using new[]. This allows compilation using clang
Remove the TeamDetectorInterface from cmpattern_teamdetector.h. The interface is not used and defines an update function that differs from the one that's actually used.
Used isnan from cmath in glcamera.h .
Fix operators in gvector.h : += would expand to x = x += p.x;. g++ complains about possibly undefined behavior. Replaced with x += p.x
In random.cpp : if (!left < 0) is always false according to clang. The correct implementation is probably `if (left < 0)

ezavesky · 2016-02-12T13:42:55Z

These look sensible. Changes like this one will help developers more easily build the app on other platforms, too.

MichaelEischer · 2016-10-16T15:13:50Z

I've rebased the original commit and added a second one that makes the codebase compatible with C++11 while staying compatible with C++98.
@joydeep-b: Any chance to merge this any time soon?

joydeep-b · 2016-10-17T15:23:21Z

@MichaelEischer have you tested this with a four-cam firewire setup to verify that the core functionality is correct?
I have a few questions regarding specific line changes, shall comment inline.

joydeep-b

Most changes look good. Good catch finding the operator bugs!
I've requested some clarification on some changes, after which this should be good to go.

joydeep-b · 2016-10-17T15:24:12Z

src/app/plugins/plugin_dvr.cpp

@@ -364,7 +364,6 @@ void PluginDVR::slotMovieSave() {
        if (output.getNumBytes() > 0) {
          //write file:
          QString num = QString::number(i);
-          int n = max(5,num.length());


Could you please explain why this is no longer required?

The variable was never read, thus no reason to calculate it. Judging from the next two lines it's actually unnecessary.

joydeep-b · 2016-10-17T15:24:30Z

src/app/plugins/plugin_legacypublishgeometry.cpp

@@ -48,8 +48,8 @@ PluginLegacyPublishGeometry::PluginLegacyPublishGeometry(
    FrameBuffer* fb,
    RoboCupSSLServer* ds_udp_server_old,
    const RoboCupField& field) :
-    _ds_udp_server_old(ds_udp_server_old),


Could you please explain why this line had to be moved?

Both clang and g++ complain when fields are not initialized in the order in which they are listed in the header file. In this case the server port was set before calling the super class constructor. As the super class constructor has no means to access this field, there is no change in behavior. Except that compiler stops complaining when -Wall is used.

joydeep-b · 2016-10-17T15:27:52Z

src/graphicalClient/soccerview.cpp

@@ -99,8 +107,7 @@ void GLSoccerView::mousePressEvent(QMouseEvent* event)

 void GLSoccerView::mouseReleaseEvent(QMouseEvent* event)
 {
-  bool shiftKey = event->modifiers().testFlag(Qt::ShiftModifier);
-  bool ctrlKey = event->modifiers().testFlag(Qt::ControlModifier);
+  (void)event;


Does removing this result in an unused variable warning? If so, what compiler and version?

The (void)event can be removed. I'm not sure whether this was the case when I've made that change.

I've update from Ubuntu 15.10 to Ubuntu 16.04 since I've made the initial version of this commit. My current compiler versions (on Ubuntu 16.04):

gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.2)

Ubuntu clang version 3.7.1-2ubuntu2 (tags/RELEASE_371/final) (based on LLVM 3.7.1)

joydeep-b · 2016-10-17T15:28:32Z

src/shared/capture/capture_generator.cpp

@@ -138,7 +138,6 @@ RawImage CaptureGenerator::getFrame()
    int slice_width = w/n_colors;
    rgb color=RGB::Black;
    rgb color2;
-    int gradient=h/256;


Is this an unused var?

joydeep-b · 2016-10-17T15:30:03Z

src/shared/capture/capturev4l.cpp

    vector<VarType *> children=item->getChildren();
    for (unsigned int i=0;i<children.size();i++) {
        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="was_read") vwasread=(VarBool *)children[i];
        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="enabled") venabled=(VarBool *)children[i];
-        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="default") vdefault=(VarBool *)children[i];


Are these lines removed since the vars are set but not used?

vauto and vdefault are only read but not used for anything else.

joydeep-b · 2016-10-17T15:31:07Z

src/shared/capture/capturev4l.cpp

    VarBool * vwasread=0;
    VarBool * vdefault=0;

    vector<VarType *> children=item->getChildren();
    for (unsigned int i=0;i<children.size();i++) {
        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="was_read") vwasread=(VarBool *)children[i];
        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="enabled") venabled=(VarBool *)children[i];
-        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="auto") vauto=(VarBool *)children[i];
-        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="default") vauto=(VarBool *)children[i];
+        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="default") vdefault=(VarBool *)children[i];


Why is "default" written (line 888), bu t not read (line 831)?

vdefault is somehow used to reset vint. Not sure what's exactly happening there.

joydeep-b · 2016-10-17T15:31:58Z

src/shared/capture/capturev4l.cpp


    vector<VarType *> children=item->getChildren();
    for (unsigned int i=0;i<children.size();i++) {
        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="enabled") venabled=(VarBool *)children[i];
-        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="auto") vauto=(VarBool *)children[i];


Could you please explain why these lines have been removed?

Those variables are once again never read. I've just noticed that these changes should be accompanied by a cleanup of the var type tree.

joydeep-b · 2016-10-17T15:33:06Z

src/shared/cmpattern/cmpattern_teamdetector.cpp

@@ -416,7 +416,7 @@ void TeamDetector::findRobotsByModel(::google::protobuf::RepeatedPtrField< ::SSL
  (void)image;
  const int MaxDetections = _other_markers_max_detections;
  Marker cen; // center marker
-  Marker markers[MaxDetections];


Why did this need to be converted from a stack allocation to a heap allocation?

clang complains otherwise:

variable length array of non-POD element type 'CMPattern::Marker'

POD is short for Plain Old Data. The Marker class has a constructor and thus doesn't count as POD.

joydeep-b · 2016-10-17T15:33:43Z

src/shared/cmpattern/cmpattern_teamdetector.h

@@ -40,14 +40,6 @@
 using namespace std;
 namespace CMPattern {

-class TeamDetectorInterface {


Why did this have to be removed?

The update function was out of sync and it's not used anywhere else.

MichaelEischer

I only have access to a 2 camera firewire setup. This should nevertheless be enough to test everything important. I should be able to test the changes tomorrow.
It would probably be a good idea to specify what should be tested and to provide a way to test most functions without requiring access to real cameras. I think that most functions could be tested by feeding four recorded camera streams into ssl-vision. That should be enough to test everything except the capture plugins.

The capturev4l changes probably need further cleanups.

MichaelEischer · 2016-10-17T17:37:26Z

src/app/plugins/plugin_dvr.cpp

@@ -364,7 +364,6 @@ void PluginDVR::slotMovieSave() {
        if (output.getNumBytes() > 0) {
          //write file:
          QString num = QString::number(i);
-          int n = max(5,num.length());


The variable was never read, thus no reason to calculate it. Judging from the next two lines it's actually unnecessary.

MichaelEischer · 2016-10-17T17:42:50Z

src/app/plugins/plugin_legacypublishgeometry.cpp

@@ -48,8 +48,8 @@ PluginLegacyPublishGeometry::PluginLegacyPublishGeometry(
    FrameBuffer* fb,
    RoboCupSSLServer* ds_udp_server_old,
    const RoboCupField& field) :
-    _ds_udp_server_old(ds_udp_server_old),


Both clang and g++ complain when fields are not initialized in the order in which they are listed in the header file. In this case the server port was set before calling the super class constructor. As the super class constructor has no means to access this field, there is no change in behavior. Except that compiler stops complaining when -Wall is used.

MichaelEischer · 2016-10-17T17:43:38Z

src/app/stacks/multistack_robocup_ssl.cpp

@@ -21,9 +21,9 @@
 #include "multistack_robocup_ssl.h"

 MultiStackRoboCupSSL::MultiStackRoboCupSSL(RenderOptions * _opts, int cameras) :
+    MultiVisionStack("RoboCup SSL Multi-Cam",_opts),


Same reason as in plugin_legacypublishgeometry.cpp

MichaelEischer · 2016-10-17T17:56:35Z

src/graphicalClient/soccerview.cpp

@@ -99,8 +107,7 @@ void GLSoccerView::mousePressEvent(QMouseEvent* event)

 void GLSoccerView::mouseReleaseEvent(QMouseEvent* event)
 {
-  bool shiftKey = event->modifiers().testFlag(Qt::ShiftModifier);
-  bool ctrlKey = event->modifiers().testFlag(Qt::ControlModifier);
+  (void)event;


The (void)event can be removed. I'm not sure whether this was the case when I've made that change.

I've update from Ubuntu 15.10 to Ubuntu 16.04 since I've made the initial version of this commit. My current compiler versions (on Ubuntu 16.04):

gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.2)

Ubuntu clang version 3.7.1-2ubuntu2 (tags/RELEASE_371/final) (based on LLVM 3.7.1)

MichaelEischer · 2016-10-17T17:57:26Z

src/shared/capture/capture_generator.cpp

@@ -138,7 +138,6 @@ RawImage CaptureGenerator::getFrame()
    int slice_width = w/n_colors;
    rgb color=RGB::Black;
    rgb color2;
-    int gradient=h/256;


MichaelEischer · 2016-10-17T18:07:28Z

src/shared/capture/capturev4l.cpp


    vector<VarType *> children=item->getChildren();
    for (unsigned int i=0;i<children.size();i++) {
        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="enabled") venabled=(VarBool *)children[i];
-        if (children[i]->getType()==VARTYPE_ID_BOOL && children[i]->getName()=="auto") vauto=(VarBool *)children[i];


Those variables are once again never read. I've just noticed that these changes should be accompanied by a cleanup of the var type tree.

MichaelEischer · 2016-10-17T18:10:49Z

src/shared/cmpattern/cmpattern_teamdetector.h

@@ -40,14 +40,6 @@
 using namespace std;
 namespace CMPattern {

-class TeamDetectorInterface {


The update function was out of sync and it's not used anywhere else.

MichaelEischer · 2016-10-17T18:13:09Z

src/shared/cmpattern/cmpattern_teamdetector.cpp

@@ -506,6 +506,7 @@ void TeamDetector::findRobotsByModel(::google::protobuf::RepeatedPtrField< ::SSL
    robots->RemoveLast();
  }

+  delete markers;


This should be delete[] . I'm a bit surprised that I didn't notice this given that clang is complaining now.

MichaelEischer · 2016-10-17T18:19:28Z

src/shared/cmpattern/cmpattern_teamdetector.cpp

@@ -416,7 +416,7 @@ void TeamDetector::findRobotsByModel(::google::protobuf::RepeatedPtrField< ::SSL
  (void)image;
  const int MaxDetections = _other_markers_max_detections;
  Marker cen; // center marker
-  Marker markers[MaxDetections];


clang complains otherwise:

variable length array of non-POD element type 'CMPattern::Marker'

POD is short for Plain Old Data. The Marker class has a constructor and thus doesn't count as POD.

MichaelEischer · 2016-10-17T18:38:37Z

src/shared/util/gvector.h

@@ -212,9 +212,9 @@ vector3d<num> cross(const vector3d<num> a,const vector3d<num> b)
  template <class num> \
  vector3d<num> &vector3d<num>::operator opr (const vector3d<num> p) \
  {                  \
-    x = x opr p.x;   \


I think it was g++ that complained about these assignments. Took me a while to understand what's happening there.

MichaelEischer · 2016-10-18T20:52:37Z

I've successfully tested the changes with a robot shooting goals in a two camera setup. Currently only a few further cleanups of the capturev4l code remain.

joydeep-b · 2016-10-19T00:39:02Z

Michael, thanks for this changeset.
Looking through the source, it seems that the v4l drivers are sorely in need of cleanup, along with some paring of the associated VarTree. I'll add that as an issue.
For now, this changeset looks good to me.

MichaelEischer force-pushed the fix-compiler-warnings branch from 38a6c53 to 0c53342 Compare October 16, 2016 15:11

This was referenced Oct 16, 2016

Fixes some memory leaks in the software #60

Merged

added high resolution timer from c++11 #56

Closed

joydeep-b reviewed Oct 17, 2016

View reviewed changes

MichaelEischer commented Oct 17, 2016

View reviewed changes

MichaelEischer force-pushed the fix-compiler-warnings branch from 69104d3 to 4eed0c7 Compare October 17, 2016 18:52

MichaelEischer added 3 commits October 18, 2016 19:49

Fix compiler warnings and default to -Wall

f873076

Fix compatibility with C++11

1065f3e

Fix marker array deletion

cb906cd

MichaelEischer force-pushed the fix-compiler-warnings branch from 4eed0c7 to cb906cd Compare October 18, 2016 17:49

joydeep-b mentioned this pull request Oct 19, 2016

V4L Code, VarTree CLeanup #61

Open

joydeep-b closed this Oct 19, 2016

joydeep-b reopened this Oct 19, 2016

joydeep-b merged commit 2ab4a28 into RoboCup-SSL:master Oct 19, 2016

MichaelEischer deleted the fix-compiler-warnings branch October 29, 2016 20:21

Fix compiler warnings and default to -Wall #53

Fix compiler warnings and default to -Wall #53

Conversation

MichaelEischer commented Feb 11, 2016

ezavesky commented Feb 12, 2016

MichaelEischer commented Oct 16, 2016

joydeep-b commented Oct 17, 2016

joydeep-b 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

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

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

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

MichaelEischer 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

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

Choose a reason for hiding this comment

MichaelEischer commented Oct 18, 2016

joydeep-b commented Oct 19, 2016