offa / scope-guard

Implementation of Scoped Guards and Unique Resource as proposed in P0052.
MIT License
46 stars 6 forks source link

Use [[nodiscard]] for classes & constructors #169

Closed martinus closed 3 years ago

martinus commented 4 years ago

Hi, first thanks a lot for this work! I've been using scope_exit in a project, and we recently had a bug because this works:

sr::scope_exit([] { /* ... */ });

It would help to mark the whole classes as [[nodiscard]], and add it to the constructors. Adding to the constructor currently produces the warning -Wattributes on g++ which has to be disabled

offa commented 4 years ago

Thanks for your report!

P0052 doesn't mention the [[nodiscard]], nor does the Library Fundamental TS v3. But from your experience it sounds reasonable.

I have pushed a branch, containing the attribute, for testing. Not only do the results differ between gcc and clang on my system, but also between their versions (see CI build; failure is intentional):

Btw. have you reported this C++ standardization or the authors already?

martinus commented 4 years ago

No, I've not reported this to anybody yet. Do you know who best to contact, and where?

offa commented 4 years ago

The best contact should be the ISO C++ mailing list I guess. There are some more infos here. I couldn't find where to contact LWG / LEWG directly.

offa commented 4 years ago

It seems the [[nodiscard]] on constructors is a defect: https://en.cppreference.com/w/cpp/language/attributes/nodiscard, related paper: P1771.

martinus commented 4 years ago

ah, that's interesting, P1771R1 talks specifically about scope_exit

It seems with that with __has_cpp_attribute(nodiscard) >= 201907 it's possible to check if ~[[noexcept]]~ [[nodiscard]] for constructors are supported: https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#attribute-feature-test-macros

offa commented 4 years ago

You mean [[nodiscard]]? Unfortunately, only clang 10 seems to get it right at the moment.

Update: According to the C++20 compiler support, GCC 10 and Clang 9/10 should support it. I'll try again with C++20 enabled.

Update: C++20 enabled doesn't make any difference.

martinus commented 4 years ago

It seems to work for me with g++10, at least a simple example. MSVC 19.24 works too, but it seems it doesn't have the right __has_cpp_attribute(nodiscard) version:

https://godbolt.org/z/bvj4zM

offa commented 4 years ago

As [[nodiscard]] should work on ordinary functions on all compiler, maybe it's viable to create some free factory functions to workaround the issue? Earlier version of the proposal included some make-functions, which got replaced by deduction guides eventually.

offa commented 3 years ago

@martinus are there any news?

martinus commented 3 years ago

Hi! I didn't follow this any more, but I saw that Peter Sommerlad, the one who wrote P0052R10 – Generic Scope Guard and RAII Wrapper for the Standard Library, also wrote the proposal "P1771R1 - [[nodiscard]] for constructors".

If I understand this correctly, [[nodiscard]] for constructors is adopted for C++20: https://github.com/cplusplus/papers/issues/526 So in future this should work.

The unique_resource though seems to not make it into C++20: https://github.com/cplusplus/papers/issues/196 I have no idea though what "#278 will continue toward LFTS3." means. Also, here is Sommerlad's scope implementation: https://github.com/PeterSommerlad/scope17

offa commented 3 years ago

Thanks for your update, so I can close here for now. Once C++20 is out (and supported by compilers), I'll add the [[nodiscard]].