gazebosim/sdformat

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

Ouverte

#1 358 ouverte le 23 janv. 2024

 (5 commentaires) (0 réaction) (1 personne assignée)C++ (122 forks)auto 404
enhancementhelp wanted

Métriques du dépôt

Stars
 (216 étoiles)
Métriques de merge PR
 (Métriques PR en attente)

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

Guide contributeur