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

[Ryanf55]

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

[mjcarroll]

I think this is a reasonable feature and can help us catch bugs sooner.

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.

We are already using C++17 in sdformat: https://github.com/gazebosim/sdformat/blob/e7d7cefc8a4775c6bf0287be06da5c4c6cf9c493/CMakeLists.txt#L39-L40

Since we already have SDFORMAT_VISIBLE and SDFFORMAT_HIDDEN, I think it would make sense to introduce a SDFORMAT_WARN_UNUSED or equivalent to wrap the [[nodiscard]] attribute, just in case there are people who don't want to use that feature (I can't imagine, other than trying to keep C++11/14 compat).

[Ryanf55]

Can do. Should I enable SDFORMAT_WARN_UNUSED depending on the value of CMAKE_CXX_STANDARD, or is there a different preferred approach?

[mjcarroll]

Something like this should work:

#if __cplusplus >= 201703L
#define SDFORMAT_WARN_UNUSED [[nodiscard]]
#else
#define SDFORMAT_WARN_UNUSED
#endif

I will bring this up in the simulation meeting today to see if anybody else has feedback on the idea/names.

[Ryanf55]

I PR'd a simple example of applying this to a public header. The value can be seen on const Trajectory *TrajectoryByIndex(uint64_t _index) const, which returns a raw pointer, with no information on whether it's an owning pointer or observer pointer. nodiscard will help ensure users don't leak data. Good thing sdformat uses smart pointers under the hood!

[mjcarroll]

The feedback from the meeting was that this is a great idea and will definitely help catch bugs.

As far as the naming, since we are already on C++17, using the [[nodiscard]] attribute as-is should be totally fine. No need for the custom macro/logic around it.