team-charls/charls

CHARLS_NO_DISCARD vs CHARLS_CHECK_RETURN

malaterre opened this issue ยท 6 comments

It would be super nice to clarify the need for CHARLS_CHECK_RETURN (now that we have [[nodiscard]]) in CharLS code base.

CHARLS_NO_DISCARD and CHARLS_CHECK_RETURN have the same intend. CHARLS_NO_DISCARD is only active when consuming the CharLS headers in C++17 mode. CHARLS_CHECK_RETURN is active for C and C++14, but only for the MSVC compiler ([[nodiscard]] requires C++17 or newer).

It would be probably possible to express CHARLS_NO_DISCARD in CHARLS_CHECK_RETURN and use only CHARLS_NO_DISCARD in the public API.
It also seems that I missed an opportunity:

RETURN_TYPE_SUCCESS_(return == 0)
enum class jpegls_errc
{
 // ...
}

can be defined as

RETURN_TYPE_SUCCESS_(return == 0)
enum class CHARLS_NO_DISCARD jpegls_errc
{
 // ...
}

If you ever keep CHARLS_CHECK_RETURN , pay attention that you have equivalent syntax for GCC/Clang:

#define CHARLS_CHECK_RETURN __attribute__ ((warn_unused_result))

[Mostly for discussion]

Reading the attributes page of GCC makes me think that one could (almost) have an equivalent to IN_ and OUT_ with:

__attribute__ ((access (read_only, 1)))

and

__attribute__ ((access (write_only, 1)))

Something like this:

#ifdef _MSC_VER
#define IN_ _In_
#define IN1_ IN_
#define IN2_ IN_
#else
#define IN1_ __attribute__ ((access (read_only, 1)))
#define IN2_ __attribute__ ((access (read_only, 2)))
...

The MSVC SAL attributes need to be in front of the parameters, the GCC attribute needs to be for or after the function.
Most functions that use pointers using IN_ or OUT_ also have already the CHARLS_ATTRIBUTE((nonnull)) for GCC/Clang.
Will experiment with the access attribute.

It seems I can't read documentation properly ๐Ÿ˜•. Indeed the 'access' macros would need to be seriously updated (take an argument)...

This issue has been resolved with bef43d5