enh(all): Add [[nodiscard]] and [[maybe_unused]] attributes

[mikomikotaishi]

Addresses part of #5152. This PR adds [[nodiscard]] attributes to many methods and types that would otherwise be important not to discard.

Unlike POCO_DEPRECATED() which can be disabled, I am not going to put [[nodiscard]] behind a macro as using deprecated symbols is intended while discarding a value is certainly a programming error.

[mikomikotaishi]

I have also moved POCO_UNUSED to [[maybe_unused]], seeing as it is a standard attribute in C++17, but I think this is probably not needed any more. POCO_UNUSED has been removed entirely as [[maybe_unused]] is a standard attribute and is portably representable.

[mikomikotaishi]

Would it be possible to get this merged before the 1.15 release? I understand if not, but it is nearly done. I have covered all methods across the project.

[github-advanced-security]

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[matejk]

Would it be possible to get this merged before the 1.15 release? I understand if not, but it is nearly done. I have covered all methods across the project.

It is quite a big change. I'll need some time for review.

[matejk]

There are lots of changes. Instead of adding comments to the source code I provide some general observations.

[matejk]

[[nodiscard]] on Enum Types

  enum [[nodiscard]] EventType { ... }
  enum [[nodiscard]] Priority { ... }
  enum [[nodiscard]] SelectMode { ... }
  enum [[nodiscard]] Storage { ... }
  enum [[nodiscard]] Allocation : unsigned char { ... }

This is unusual and mostly useless. [[nodiscard]] on an enum means functions returning that enum type should have their return value checked. But enum values are typically used as parameters, not return values. The attribute on the enum declaration itself doesn't do anything for enum values.

[matejk]

[[nodiscard]] on Internal/Helper Classes

  class [[nodiscard]] FunctorRunnable: public Runnable { ... }
  class [[nodiscard]] ValueHolder { ... }
  class [[nodiscard]] Block { ... }
  class [[nodiscard]] FDCompare { ... }

These are internal classes that users don't directly construct or receive from functions. Adding [[nodiscard]] adds noise without benefit.

[matejk]

[[nodiscard]] on Dereference Operators

  [[nodiscard]] C* operator -> ()
  [[nodiscard]] C& operator * ()

This is wrong. These operators are used for accessing members:

  ptr->doSomething();  // This would warn! The return of -> is "discarded"

The -> operator result IS used implicitly. This will likely cause spurious warnings.

[matejk]

[[nodiscard]] on Nullable and Optional Classes

  class [[nodiscard]] Nullable { ... }
  class [[nodiscard]] Optional { ... }

Reconsider this. While it ensures you use the object, these types are commonly constructed just to be assigned to variables or passed around. The standard library's std::optional is not marked nodiscard.

[mikomikotaishi]

I decided to put nodiscard on enums and class internal types because I wanted to ensure that data like this is not discarded.

I have gone ahead and applied your requested changes, nonetheless.

[matejk]

@mikomikotaishi , you don't have to force push. It is harder to track changes between the commits when force pushed.

[mikomikotaishi]

Noted

[andrewauclair]

While it ensures you use the object, these types are commonly constructed just to be assigned to variables or passed around.

Not sure what you mean by this, but [[nodiscard]] on the enum or class declaration only applies to functions returning that type by value.

[mikomikotaishi]

Is there anything else I should address in this PR, pending reviews from the other reviewers?

[matejk]

Build with enabled compiler warnings shall be inspected whether introduction of attributes introduced new warnings and then decide how to fix those warnings:

• change the code at the warning point
• revert introduction of the attribute

Two jobs with enabled warnings are built on github: macos-clang-cmake-warnings and macos-msvc-cmake-warnings.

[mikomikotaishi]

Apologies, accidentally force-pushed instead of creating a new commit. (I'll force-push when making amendments from now on and use proper commits when adding something new.)

Anyway, I think I've done what I can to remove the warnings we were getting.

[mikomikotaishi]

Never mind, I see the problem now, I accidentally overwrote the handled variable.

[mikomikotaishi]

@matejk Is there anything to do next?