ethz-asl / programming_guidelines Goto Github PK

1. MyClass.hpp
2. myclass.hpp
3. my_class.hpp

https://github.com/ethz-asl/programming_guidelines/wiki/Catkin-Eclipse in
https://github.com/ethz-asl/programming_guidelines/wiki#ide-integration
Maybe you'd want to review.
Thanks @markusachtelik , @simonlynen for the input to this.

I don't agree with the wiki text:
"Note that structs are kept in C++ for compatibility with C only, and avoiding them increases the readability of the code by reducing the number of constructs used. Use a class instead."

I would prefer using structs as pure data structs and classes in all other cases

for 32 bit floating point values, writing float x = 0.0 can lead to performance issues especially for embedded platforms, since apparently a double is assumed (someone told me it happened on a cortex), so float x = 0.0f should be preferred.

I currently wander what to do with unused parameters. Especially when it comes to overriding they are clearly unavoidable. But the compiler warning seem quit e useful (enabled implicitly with -Wextra)

With gcc there seem to be three options :

1. have no name for the unused parameters
   Contra: bad when you want to start using it, maybe irritating when reading
   Pro: no macro needed (to create compiler independent code)
2. declare it as unused (see http://stackoverflow.com/questions/3599160/unused-parameter-warnings-in-c-code for ideas)
   Contra: need macro and that macro needs a place (header file ...)
   Pro: can be nice to read and self documenting
3. -Wno-unused-parameter
   Contra: killing useful warning
   Pro: simple

What do you think / how do you currently handle that?

How do you do it? If I do

catkin build --merge-devel --force-cmake --cmake-args -G"Eclipse CDT4 - Unix Makefiles"

I get:

[nlopt] ==> '/home/titus/catkin_ws/build/nlopt/build_env.sh /usr/bin/cmake /home/titus/catkin_ws/src/nlopt -DCATKIN_DEVEL_PREFIX=/home/titus/catkin_ws/devel -DCMAKE_INSTALL_PREFIX=/home/titus/catkin_ws/install -GEclipse CDT4 - Unix Makefiles' in '/home/titus/catkin_ws/build/nlopt'
CMake Error: Could not create named generator Eclipse
[nlopt] <== '/home/titus/catkin_ws/build/nlopt/build_env.sh /usr/bin/cmake /home/titus/catkin_ws/src/nlopt -DCATKIN_DEVEL_PREFIX=/home/titus/catkin_ws/devel -DCMAKE_INSTALL_PREFIX=/home/titus/catkin_ws/install -GEclipse CDT4 - Unix Makefiles' failed with return code '1'

Probably due to:
http://stackoverflow.com/questions/16073040/call-cmake-from-python-script-results-in-could-not-create-named-generator

I currently found a workaround by:

cd build
cmake ../src/ -DCATKIN_DEVEL_SPACE=../devel -DCMAKE_INSTALL_PREFIX=../install -G"Eclipse CDT4 - Unix Makefiles"

As inspired by:
http://catkin-tools.readthedocs.org/en/latest/commands/catkin_build.html?highlight=cmake#understanding-the-build-process

The guidelines are gold when working with ROS. I think it would be worth pushing them to the ROS wiki. I especially like the Qt Cretor guidelines. E.g. the CMake fix also applied for CLion.

ROS and google are quite similar.
http://google-styleguide.googlecode.com/svn/trunk/eclipse-cpp-google-style.xml
http://www.ros.org/wiki/IDEs?action=AttachFile&do=view&target=ROS_format.xml

Main difference:
Indentiation in spaces: ROS: 4, Google: 2 (I vote for Google)
Curly bracket on new line for conditionals: ROS: yes, Google: no (I vote for Google)
Initializer list break: ROS: no, Google: yes (I vote for ROS)

ROS:

/*
 * A sample source file for the code formatter preview
 */
#include <math.h>

class Point
{
public:
  Point(double x, double y) :
      x(x), y(y)
  {
  }
  double distance(const Point& other) const;

  double x;
  double y;
};

double Point::distance(const Point& other) const
{
  double dx = x - other.x;
  double dy = y - other.y;
  return sqrt(dx * dx + dy * dy);
}

I would vote for Google which formatted the following example:

/*
 * A sample source file for the code formatter preview
 */
#include <math.h>

class Point {
 public:
  Point(double x, double y)
      : x(x),
        y(y) {
  }
  double distance(const Point& other) const;

  double x;
  double y;
};

double Point::distance(const Point& other) const {
  double dx = x - other.x;
  double dy = y - other.y;
  return sqrt(dx * dx + dy * dy);
}

Awesome style guide, thanks! Here are some points which I'd like your input on and could maybe made explicit in the style guide.

• Should one always use setter and getter methods?
• Duplicate code is evil! Create functions for commonly used commands (also for simple ones that become long because of long variable names).
• Write code to be as less error prone as possible (How to we achieve this?)
• A class should be understandable/usable by reading the public interface and without having to read all the code.

I guess these points are pretty clear to all experienced programmers. But some of them could maybe be phrased more explicit in the guide for the beginners.

Why a basic indentation of 2? Why indenting with spaces and not tabs?

Should this be called differently, something along these lines?

getRef()
ref()

As for 55, it makes sense for pointers to test for non 0 using

int* p(0);
if (p)
    ...

So I do no agree with 70. I find that C++ is already quite verbose, and making it less verbose while keeping the code easy to read should be a target.

There seems to be a problem affecting some repositories.
At least:
aslam_nonparametric_estimation (see e.g. http://129.132.38.183:8080/job/aslam_nonparametric_estimation/254/consoleText - at the very end)
aslam_simulation
aslam_map_manager
??

I believe this is a common reason for those not to become green anymore!

Where do we put build server related issues?

We use shared pointers for memory management but I still have some questions on what is the best use.

Let's say I have a class, Frame, that has a shared_ptr member Camera. The camera is shared among many frames. I have some questions about best practices:

class Frame {
public:
  // Q1
  Frame(std::shared_ptr<Camera> camera) : camera_(camera) {}

  // Q2
  std::shared_ptr<Camera> getCamera() const{ return camera_; }

  // Q3
  void setCamera( std::shared_ptr<Camera> camera ) { camera_ = camera; }
private:
  std::shared_ptr<Camera> camera_;
};

Questions:

Q1. What should the constructor take as an argument: std::shared_ptr<Camera>, or const std::shared_ptr<Camera>&, or something else?

Q2. What should getCamera() return, std::shared_ptr<Camera>, or const std::shared_ptr<Camera>&, or const std::shared_ptr<const Camera>&, or something else? Or should we have a const and a non-cost version. And should we name the non-const version getCameraMutable()?

Q3. What should setCamera() take as an argument: std::shared_ptr<Camera>, or const std::shared_ptr<Camera>&, or something else?

It should include stuff like

1. how to do branching
2. what to name branches (fix/xxx, feature/yyy, or something)
3. to keep binary files out of code repos,
4. managing a workspace using aslam_status

Other stuff?

Hi,
You have a very nice example for using boost/python and ros. I wanted to ask if it is also usable for interfacing a c++ script in ros that has nodehandle. I have made a ros_control hardware interface and want to now interface it with a python simulator. My choices seems to be Cython, boost/python, swig, cffi and etc.
Here I noticed a possible solution with boost/python and they mention something specific about having nodehandle and my other question was if you may think there might be an advantage in using cython compared to boost/python.

Thanks.

Alternatives:

1. Named constants (including enumeration values) must be all uppercase using underscore to separate words. DAYS_IN_A_WEEK
2. Mixed case starting with upper case: DaysInAWeek (ROS)
3. Use a k followed by mixed case: kDaysInAWeek. (Google)

What about inlining?

If inlining is desired, the respective function definition should be moved to a separate implementation header which is included at the end of the header.

What about python?
I prefer suffix underscore

• structure the guide so important things come to the front. make it easier to read for the students.
• const-correctness -> make as many methods and parameters const as possible
• parameter passing by reference to const (motivate with cost of copy)
• no using directives for namespaces in the global scope

The guide should provide modelines for common editors (vim, katepart, emacs, eclipse) so that the editor will use the correct settings. In addition, the guide should specify that when an incompatible setting is used, the corresponding source code should provide a modeline to avoid the editor fighting against the different convention.