Consider [[nodiscard]] on all public C++ getters to prevent usage bugs
#1,358 opened on Jan 23, 2024
Repository metrics
- Stars
- (216 stars)
- PR merge metrics
- (PR metrics pending)
Description
Current behavior
The following code compiles with default compiler settings when building a camera plugin in Gazebo:
void MyCameraPlugin::PreUpdate(
const UpdateInfo &_info,
EntityComponentManager &_ecm)
{
// All the boilerplate to get the SDF object
Entity cameraEntity = this->impl->cameraSensorEntity;
auto comp = _ecm.Component<components::Camera>(cameraEntity);
if (!comp)
return;
sdf::Sensor &sensor = comp->Data();
sdf::Camera *cameraSdf = sensor.CameraSensor();
if(!cameraSdf)
return;
// Now, at some point during an algorithm:
cameraSdf.ImageWidth(); // A bug!
}
Calling any of the "getters" in gs/sdformat14/sdf/Camera.hh without assigning it to a value would be a bug.
Desired behavior
C++17 can protect you against this with a compiler directive called [nodiscard].(https://en.cppreference.com/w/cpp/language/attributes/nodiscard) A great talk on the value of this compiler directive is seen here: https://youtu.be/teUA5U6eYQY?si=GluASrB8dnTNf0HM&t=1602
If, instead the header had the following, the compiler will warn if you forget to assign the return value. Thus the compiler catches a bug while you are developing code, or in CI if you have warnings treated as errors.
public: [[nodiscard]] uint32_t ImageWidth() const;
Alternatives considered
- Strict code reviews on projects that can't use C++17.
Implementation suggestion
Adding [[nodiscard]] may introduce warnings on previously compiling code. Yes, these would all be bugs anyways, but I would be hesitant to pull this into SDF14. I suggest considering it for the next version of SDF.
I wasn't able to find anywhere that specified the C++ standard used for the public header of sdformat, but nodiscard is C++17 or above. If SDFormat is intended to be used in C++11/C++14 environments, it could be wrapped and hidden by the compiler optionally.
I would be willing to help implement it.
Tooling
Clang-tidy may be able to do it automaticlly: https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/modernize-use-nodiscard.html