RFC: Proposal to reconcile ROS 1 and ROS 2 C++ style guides

marguedas · 2018-04-25T15:23:40.779Z

Hello ROS users!

We’ve been starting to migrate some core packages to ROS 2 and in the process found a few conflicts between the ROS 1 and ROS 2 C++ style guides.
These differences cause some friction when either trying to have a single branch be both ROS 1 and ROS 2 or when just maintaining patches between the ROS 1 and ROS 2 versions.
So, in order to minimize this friction, we’d like to eliminate any conflicts that we can.
After looking at the conflicts, it seems that relaxing the ROS 1 style’s in a few places is the best solution, but we would like some feedback on the proposed changes.
All of the changes would not require any changes in existing ROS 1 packages, but would allow ROS 1 packages to make changes to their style to comply with both the ROS 2 style and the newly-relaxed ROS 1 style.

These are the changes we’re proposing to the ROS 1 C++ style guide:

Header file names:
- Existing ROS 1 style: header files should always use the .h extension
- Proposed ROS 1 style: header files should use either .h or .hpp, if you are planning on supporting both ROS 1 and ROS 2, the .hpp extension is recommended.
  - Rationale: differentiating between C and C++ header files provides a way to have both a C and a C++ interface to a library without having to modify the filenames arbitrarily
  - Additional benefit: it gives humans and tools a hint about which language is used in the given header file
Header define guards:
- Existing ROS 1 style: <PACKAGE>_<PATH>_<FILE>_H
- Proposed ROS 1 style: <PACKAGE>[_]_<PATH>[_]_<FILE>_{H,<EXT>}[_] and optionally leave <PACKAGE> off when <PATH> is the same
  - Allow an optional trailing underscore
    - Rationale: matches Google C++ style guide recommendation (i.e. <PROJECT>_<PATH>_<FILE>_H_)
  - Allow for double underscore as separator between PATH tokens, the PROJECT token, and the FILE token of the guard name
    - Rationale: reduces the chance of guard name collision, though it doesn’t prevent it, see: https://github.com/google/styleguide/issues/78
  - Optionally use actual file extension, e.g. if the header is pluginlib/pluginlib.hpp then use PLUGINLIB__PLUGINLIB_HPP_ rather than PLUGINLIB__PLUGINLIB_H_
    - Rationale: allows for coexisting C and C++ headers to have different guard names
Braces:
- Existing ROS 1 style: open braces (i.e curly braces should always be on their own line)
- Proposed ROS 1 style: allow any consistent braces style, but continue to recommend open braces for ROS 1 only code (i.e. don’t change unless trying to coexist with other style guides)
  - Rationale: This is the most subjective change, but by relaxing the requirement in the ROS 1 guide it allows us to have the ROS 1 style coexist with the ROS 2 style and others more easily.
    - Closer to Google C++ Style Guide recommendation: e.g. “The open curly brace is always on the end of the last line of the function declaration, not the start of the next line.”, but it defines brace style for each case separately.
    - Minimizes vertical whitespace use: The more code that fits on one screen, the easier it is to follow and understand the control flow of the program.
    - A preference was given to use the newer ROS 2 style in “dual-homed” packages and add an option to the ROS 1 style checkers to ignore the brace style violations, though the inverse is also possible.

These are our preferred changes, but we’re interested in the feedback from everyone.

Once we reach some consensus we’ll help update any ROS 1 linters with changes and/or options.

Your friendly ROS team

davetcoleman · 2018-04-25T16:52:46.836Z

+1 moving towards Google style guidelines even in ROS 1. I’d also hope that in ROS2 we don’t deviate much if at all for Google’s guidelines… it looks like that has already begun with the ROS2 list of “exceptions” to the Google style.

gbiggs · 2018-04-27T00:36:05.201Z

For the braces, should the proposed ROS 1 style include an explicit note about using the ROS 2 style if code is expected to support ROS 2 as well, like the header file recommendation does?

flixr · 2018-04-27T08:06:28.238Z

+1 for the ROS 2 style in ROS 1