uwreact / frc_control

ROS metapackage enabling FRC teams to use ROS on their robots

BSD 3-Clause "New" or "Revised" License

5 stars 0 forks source link

Update C++03 packages to C++14 #41

Closed matthew-reynolds closed 5 years ago

matthew-reynolds commented 5 years ago

Pull Request

Now that we're on Melodic, the default C++ standard is C++14, rather than C++03 in Kinetic. This PR just updates a few small things to bring the packages up to modern C++.

(I know #pragma once isn't standard and isn't part of C++11 or 14, but it's pretty fair to lump it with modern C++)

Contribution Checklist

[x] I have read the contributing guide.
[ ] I have referenced relevant issues.
[x] I have labeled accordingly.
[x] I have detailed my changes as much as possible.

Change Checklist

[ ] I have added tests.
[x] I have added necessary documentation.
[x] I have auto formatted using clang-format or yapf.

wmmc88 commented 5 years ago

Travis failing on clang format

matthew-reynolds commented 5 years ago

Updated - Apologies for slow turnaround.

wmmc88 commented 5 years ago

Because pragma once is not standard, should we keep the include guards just in case? Supposedly g++ does the same optimization when include guards are detected.

matthew-reynolds commented 5 years ago

You're right, these days most compilers (At least GCC) are smart enough to perform the same optimizations with include guards and pragma once.

Pragma once is supported by every worthwhile compiler and is easier to maintain, I would prefer to keep it unless there are compelling arguments otherwise. The biggest one people point out is that including the same file from different relative paths can cause issues in some cases, but that isn't really a concern the way Catkin does things - We always #include <package_name/header.h> rather than using relative paths.

(Also I'm pretty sure a lot of ROS pkgs use #pragma once so if we're worried about portability I think we're toast anyways.)