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?

[matejk]

I am quite busy with other projects currently. I'll take a look ASAP. Thank you so much for your contribution so far.

[mikomikotaishi]

Hi @matejk, is there still any time to eventually review and merge this?

[matejk]

Hi, sorry for not answering. We are preparing release 1.15.0. Changes in this PR will be included in the next release.

Thank you for your patience.

[mikomikotaishi]

So would it be possible to roll out this in a minor release (like 1.15.1)?

[mikomikotaishi]

Now that 1.15.0 is rolled out, could we look towards integrating this for 1.15.1 perhaps?

[mikomikotaishi]

@matejk also I think the website needs to be updated for 1.15.0, it still says 1.14.2

[mikomikotaishi]

I'd also like to ask here (before making an issue that might just be not worth investigating): is there any reason why we primarily use unscoped C-style enums rather than enum class? If possible I think it's worthwhile migrating to enum class, and making it an option during compilation (perhaps remaining with enum would be a better default choice until later on when it could be deprecated).

[matejk]

I'd also like to ask here (before making an issue that might just be not worth investigating): is there any reason why we primarily use unscoped C-style enums rather than enum class? If possible I think it's worthwhile migrating to enum class, and making it an option during compilation (perhaps remaining with enum would be a better default choice until later on when it could be deprecated).

The reason is very simple: lots of Poco code was written before C++11 standard. Probably some of the enums could be converted to enum class. Probably not all.

[matejk]

Now that 1.15.0 is rolled out, could we look towards integrating this for 1.15.1 perhaps?

We'll include into 1.16.0 with the next round of C++ modernisation.

[andrewauclair]

I'd also like to ask here (before making an issue that might just be not worth investigating): is there any reason why we primarily use unscoped C-style enums rather than enum class? If possible I think it's worthwhile migrating to enum class, and making it an option during compilation (perhaps remaining with enum would be a better default choice until later on when it could be deprecated).

As a user of Poco and one who has done these types of changes, I'd suggest a middle ground approach. It's more forgiving on user code to use enum class in combo with using enum. That gives proper type checking without a crap ton of errors and code changes for the scoping part.

[mikomikotaishi]

We'll include into 1.16.0 with the next round of C++ modernisation.

Could this still be merged for the time being or will we have to wait until work on 1.16.0 begins before merging this into the library?

That gives proper type checking without a crap ton of errors and code changes for the scoping part.

I agree, I think the primary goal of moving towards enum class is at having the benefits of type safety. Perhaps it could be a compile option on whether to enable using enum (by default it should be on for backwards compatibility, but I think it's better to move towards eventually deprecating it with using).