gazebosim / gazebosim/sdformat

Consider [[nodiscard]] on all public C++ getters to prevent usage bugs

Open
#1,358 5 comments 0 reactions 1 assignee Claimed by @Ryanf55 View on GitHub
enhancement help wanted
Dominant language
C++
Stars
216
Forks
125
Avg merge
1d 14h
Merged PRs (30d)
14

Description

## Current behavior

The following code compiles with default compiler settings when building a camera plugin in Gazebo:
```c++
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(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.
```c++
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

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.