Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added fallthrough macro to suppress C++17 fallthrough warnings on MSVC and other compilers #1290

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wmcnamara
Copy link

This PR adds a STBI_FALLTHROUGH macro to fix warnings on Visual Studio about switch fallthrough.

The STBI_FALLTHROUGH macro is written to be defined if C++17 is enabled, and if it is enabled it will define STBI_FALLTHROUGH as [[fallthrough]], which stops the warnings.

@Wohlstand
Copy link

Wohlstand commented Feb 18, 2022

Please keep comment lines (you may append macro being before comment, but don't remove comments), they are required to shut up the GCC 7+ and CLang warnings (even with older standards than C++17, I do have C++11 at my projects, and this warning always pops up if don't add the "// fallthrough" or "/* fallthrough */" comment line)

@wmcnamara
Copy link
Author

Thank you for catching this! I had no idea. I've added the comment to the end of the macro!

@Wohlstand
Copy link

Wohlstand commented Feb 18, 2022

Lol, not that I actually meant, you just now made that both [[fallthrough]] and comment will appear at C++17 and some special conditions. I meant, comments should be always, at all standards. So:

//C++17 fallthrough macro
#if (__cplusplus >= 201703L) || (defined(_MSVC_LANG) && (_MSVC_LANG >= 201703L) && (_MSC_VER >= 1913)) 
#define STBI_FALLTHROUGH [[fallthrough]];         // <------ Keep this as-is
#else
#define STBI_FALLTHROUGH
#endif

....
switch(x)
{
case 1:
case 2: STBI_FALLTHROUGH /* fallthrough */         //  <------------ Comment must be always at the here
case 3:
}

@Wohlstand
Copy link

P.S. I doubt comments will be assed by macros, will need to run the preprocessor and see what it actually generated on output...

@wmcnamara
Copy link
Author

Oops, yeah I wasnt really thinking about that when I wrote it...

I'll go ahead and change that

@Wohlstand
Copy link

I did an exact test:

With GCC:
изображение

With CLang:
изображение

So, adding that comment at macro has absolutely no sense

@Wohlstand
Copy link

Now looks way better 🦊 👍

@moon-chilled
Copy link

It seems redundant to have both the comment and the explicit declaration. Why not this instead?

#elif defined(__GNUC__)
# define STBI_FALLTHROUGH __attribute__((fallthrough))

@wmcnamara
Copy link
Author

Thats definitely a much nicer solution.

@Wohlstand Does this fix the issue aswell on your end?

@Wohlstand
Copy link

It seems redundant to have both the comment and the explicit declaration. Why not this instead?

Old compilers would get hurt (such as GCC 5 and below), gonna check some to make sure all clear.

@Wohlstand
Copy link

Wohlstand commented Mar 11, 2022

On GCC 4.8 the next warning appears:

# gcc -c fallthrough.c -o fallthrough.o
fallthrough.c: В функции «main»:
fallthrough.c:11:9: предупреждение: пустая декларация [по умолчанию включена]
         __attribute__((fallthrough));
         ^

"Empty declaration"

p.s. This code I tried to compile:

#include <stdio.h>

int main()
{
    int cond = 42;

    switch (cond)
    {
    case 42:
        printf("Hello");
        __attribute__((fallthrough));
    case 43:
        printf(" world!\n");
    }

    return 0;
}

on GCC 9 it doesn't gives any warnings

@moon-chilled
Copy link

I think it is reasonable to give worse diagnostics on older compiler versions. But yeah there should be a check for minimum gcc version.

@Wohlstand
Copy link

So, the best and most universal solution would be just keeping the /*fallthrough*/ comment which doesn't break old compilers and is approvable by modern compilers to not have the warning here. Speaking about MSVC compilers, they were the most painful compared to others (exclusively everything older than MSVC2015). There is still be the annoying case against friend templates in classes and the painful inability to take the pointer to the member of the sub-structure (everything works, but MSVC just fails).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants