Use [[nodiscard]] on ScopeGuard

[msimberg]

Unsure if I'm supposed to include Kokkos_Macros.hpp directly for the macro or if it's enough to get it through Kokkos_Core_fwd.hpp.

[masterleinad]

I'm not sure how this would help with the most common use case/user error, see https://godbolt.org/z/EfvGba7K6.
We would only warn for a function returning ScopeGuard and not storing its return type but constructing an unnamed object of type ScopeGuard wouldn't raise a warning.

[msimberg]

Right, it's unfortunately compiler-specific how well the attribute is taken into account. clang uses it for warnings even if the attribute is only used at the class definition. I could also mark the constructors [[nodiscard]], which seems to also make GCC warn about the mistake that you're talking about.

[masterleinad]

It seems to help at least with clang, see https://godbolt.org/z/Tv49qnPT8.

[masterleinad]

Having also the constructor as [[nodiscard]] indeed seems to help at least for clang++, g++, and msvc, see https://godbolt.org/z/absMzfoW7. Please add it there, too.

[msimberg]

Having also the constructor as [[nodiscard]] indeed seems to help at least for clang++, g++, and msvc, see https://godbolt.org/z/absMzfoW7. Please add it there, too.

Force-pushed to also mark the constructors [[nodiscard]].

[masterleinad]

Thanks!

[dalg24]

One of the CUDA builds yields bogus warnings

/var/jenkins/workspace/Kokkos/core/src/Kokkos_Core.hpp(200): warning: the "nodiscard" attribute doesn't apply to constructors, destructors, or routines with void return type

/var/jenkins/workspace/Kokkos/core/src/Kokkos_Core.hpp(208): warning: the "nodiscard" attribute doesn't apply to constructors, destructors, or routines with void return type

/var/jenkins/workspace/Kokkos/core/src/Kokkos_Core.hpp:200:57: error: 'nodiscard' attribute applied to 'Kokkos::ScopeGuard::ScopeGuard(int&, char**)' with void return type [-Werror=attributes]
   KOKKOS_ATTRIBUTE_NODISCARD ScopeGuard(int& argc, char* argv[]) {
                                                         ^
/var/jenkins/workspace/Kokkos/core/src/Kokkos_Core.hpp:209:36: error: 'nodiscard' attribute applied to 'Kokkos::ScopeGuard::ScopeGuard(const Kokkos::InitializationSettings&)' with void return type [-Werror=attributes]
       const InitializationSettings& settings = InitializationSettings()) {
                                    ^

Withdrawing until issue with warnings is resolved

[masterleinad]

According to https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#nodiscard, we should use

#if __has_cpp_attribute(nodiscard) >= 201907

[msimberg]

According to https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#nodiscard, we should use

#if __has_cpp_attribute(nodiscard) >= 201907

Thanks for looking that up. Pushed that change but I haven't checked locally if nvcc reports that correctly...

[msimberg]

Guarded by both KOKKOS_ENABLE_CXX17/20 and __has_cpp_attribute because of https://github.com/msimberg/kokkos/runs/7390818883?check_suite_focus=true#step:4:12.

In principle there could be also a separate macro to use the 201603 version of nodiscard outside of constructors, but do you think that would be useful?

[masterleinad]

In principle there could be also a separate macro to use the 201603 version of nodiscard outside of constructors, but do you think that would be useful?

I think we should also provide the macro when __has_cpp_attribute(nodiscard) is less than 201907 (but don't use it for constructors).

[msimberg]

In principle there could be also a separate macro to use the 201603 version of nodiscard outside of constructors, but do you think that would be useful?

I think we should also provide the macro when __has_cpp_attribute(nodiscard) is less than 201907 (but don't use it for constructors).

What I meant is that one could have:

#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard) >= 201603
#define KOKKOS_ATTRIBUTE_NODISCARD [[nodiscard]]
#else
...
#endif

#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard) >= 201907
#define KOKKOS_ATTRIBUTE_NODISCARD_CONSTRUCTOR_OR_2019_OR_SOME_OTHER_APPROPRIATE_NAME [[nodiscard]]
#else
...
#endif

That would avoid having the #ifdef at the constructor definition and is a bit nicer if nodiscard is useful in other classes in Kokkos as well. On the other hand, it requires knowing that there are two versions of the macro and one can still accidentally use the first macro on constructors (though CI and warnings would hopefully make sure using the wrong macro would not end up on develop/master).

[masterleinad]

We will require C++17 after the 3.7 release anyway and can then replace the current KOKKOS_ATTRIBUTE_NODISCARD with [[nodiscard]] (and possibly get rid of the macro altogether). Thus, I don't think there is much of a point in modifying the macro.

I'm fine with adding

#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard) >= 201907
#define KOKKOS_IMPL_ATTRIBUTE_NODISCARD_CONSTRUCTOR [[nodiscard]]
#else
#define KOKKOS_IMPL_ATTRIBUTE_NODISCARD_CONSTRUCTOR
#endif

if you prefer that over having the condition at both constructors but wouldn't want to add a macro that users would be encouraged to use downstream.

[msimberg]

We will require C++17 after the 3.7 release anyway and...

Fair enough. In that case it's simpler to just put the ifdefs at the constructors. I've updated this accordingly.