nemequ / hedley

A C/C++ header to help move #ifdefs out of your code
https://nemequ.github.io/hedley/
Creative Commons Zero v1.0 Universal
774 stars 51 forks source link

Annotation for maybe unused code is missing #45

Closed kayhayen closed 3 years ago

kayhayen commented 3 years ago

Hello,

I am the author of Nuitka a Python to C11 transpiler, and use macros in it a lot. I only now discovered Hedley when trying to checkout if there was improvements to likely and unlikely available, and indeed I found this.

What I came across going over my include file is this:

/* A way to not give warnings about things that are declared, but might not
 * be used like in-line helper functions in headers or static per module
 * variables from headers.
 */
#ifdef __GNUC__
#define NUITKA_MAY_BE_UNUSED __attribute__((__unused__))
#else
#define NUITKA_MAY_BE_UNUSED
#endif

Frequently in code generation, I might declare things that end up being unused and will then be picked up by the C compiler. It is however nothing like this in hedley. As you can see, this would very well need more compiler support, and fall into the category of what hedley does, but I didn't find anything that ressembles it or comes close to it. Do you think this can be added into the scope, or do you think it's nonsense, because normal code doesn't want to have this.

nemequ commented 3 years ago

This is definitely something that, in concept, would be appropriate for Hedley. Hovewer, the reason it's not included is that there is a better (more portable) alternative: cast to void. For example, instead of

void foo(__attribute__((__unused__)) int a) { }

You can do

void foo(int a) {
  (void) a;
}

Casting to void works in every compiler I'm aware of which has the ability to issue a diagnostic about an unused variable. While the unused attribute does have very good compiler support (off the top of my head, I know gcc, clang, armcc, craycc. It doesn't work in MSVC, IAR, or suncc. I'd have to check other compilers. There is also a [[maybe_unused]] attribute we could use to implement this in C++17.

A few compilers do have compiler-specific ways of informing them that a variable may be unused, but we couldn't really wrap them up in a macro and have them work. For example, suncc has a lint directive which can be used, but it requires the parameter name (NOTE(ARGUNUSED(a))).

Casting to void, OTOH, should work everywhere. While the C standard doesn't explicitly mention unused diagnostics, § 6.3.2.2 (of C17) gets pretty close:

If an expression of any other type is evaluated as a void expression, its value or designator is discarded. (A void expression is evaluated for its side effects.)

So, if I were going to add a HEDLEY_UNUSED macro, I think the best implementation would be #define HEDLEY_UNUSED(x) ((void) (x)), and at that point having a macro at all seems rather pointless since people can just cast to void themselves and it would not only save a few characters, but also not require people to learn a new macro.

I'm going to close this issue for now, but if you feel like there is a use case for what you propose that isn't satisfied by casting to void please feel free to re-open. This is definitely something I could be convinced to add if there is a good reason, I just don't see one.

kayhayen commented 3 years ago

That works for local variables, but how about code like this:

NUITKA_MAY_BE_UNUSED static PyObject *GET_DICT_ENTRY_VALUE(Nuitka_DictEntryHandle handle) { return handle->me_value; }

This like many helper functions, is defined in a header included, any may or may not be used. I string doubt the cast works there, does it? So are you suggesting to do something like this:

static PyObject *GET_DICT_ENTRY_VALUE(Nuitka_DictEntryHandle handle) { return handle->me_value; }
(void) GET_DICT_ENTRY_VALUE;

But mind you, this is not in a scope, so that wouldn't work as well.

I am using the same approach with local variables too, where I am too lazy to determine ahead of time, if a variable declaration is really needed, leaving that the C compiler to remove it, but to warn about cases where I know it ought to be used, due to -Wall in my debug mode.

nemequ commented 3 years ago

For unused functions I think the best option is to (temporarily) disable that diagnostic. The unused attribute will work on GCC and clang (as well as clang-derived compilers), but AFAIK on MSVC there is no declspec which is similar to the unused attribute, so MSVC users would still see a diagnostic.

On GCC and clang the relevant diagnostic is -Wunused-function, on MSVC it's C4505. I could add a HEDLEY_DIAGNOSTIC_DISABLE_UNUSED_FUNCTION to Hedley, something like

#if HEDLEY_HAS_WARNING("-Wunused-function")
  #define HEDLEY_DIAGNOSTIC_DISABLE_UNUSED_FUNCTION _Pragma("clang diagnostic ignored \"-Wunused-function\"")
#elif HEDLEY_GCC_VERSION_CHECK(3,4,0)
  #define HEDLEY_DIAGNOSTIC_DISABLE_UNUSED_FUNCTION _Pragma("GCC diagnostic ignored \"-Wunused-function\"")
#elif HEDLEY_MSVC_VERSION_CHECK(19,0,0) /* Likely goes back further */
  #define HEDLEY_DIAGNOSTIC_DISABLE_UNUSED_FUNCTION __pragma(warning(disable:4505))
#else
  #define HEDLEY_DIAGNOSTIC_DISABLE_UNUSED_FUNCTION
#endif

So you could just do

HEDLEY_DIAGNOSTIC_PUSH
HEDLEY_DIAGNOSTIC_DISABLE_UNUSED_FUNCTION

/* Functions go here */

HEDLEY_DIAGNOSTIC_POP

Obviously you could get away without the push/pop, but in a header (especially a public one) that's probably not a good idea.

Does that sound reasonable to you? I'd want to do some more testing in MSVC since the version check above is probably a bit too conservative, but other than that it should be ready to go.