gazebosim/sdformat

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

Open

#1,358 opened on Jan 23, 2024

View on GitHub
 (5 comments) (0 reactions) (1 assignee)C++ (122 forks)auto 404
enhancementhelp wanted

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

Contributor guide