Use gcc broken friend workaround also with clang8

[tcamargo]

"Fix"

In file included from ../../../../../src/emu/emu.h:103:
/Users/buildbot/buildbot/osx/libretro-mame/build/projects/retro/mame/gmake-osx-clang/../../../../../src/emu/devcb.h:1102:12: error: 'consume' is a protected member of 'devcb_write<unsigned char, '\xFF'>::builder_base'
                { m_sink.consume(); }
                         ^

full build log: http://paste.libretro.com/215612

Feedback welcome about best way to implement the compiler check.

[rb6502]

I'd prefer completely separate GCC and Clang checks, but this is fine otherwise.

[tcamargo]

@rb6502 Do you mean I should replicate the whole block for clang?

[smf-]

I don't think it's actually right, the code before wanted to be sure it had GCC before checking the version number. #if defined(__GNUC__) && !defined(__clang__) #if __GNUC__ >= 8 latest clang on windows currently sets __GNUC__ to 4, but they could increase it to 8 tomorrow. So checks for specific versions of __GNUC__ should always exclude clang. But now it's always going to look at whatever clang sets __GNUC__ to before it checks the __clang_major__ #if defined(__GNUC__) || defined(__clang_major__) #if (__GNUC__ >= 8) || (__clang_major__ == 8) I think you would want something like this. #if (defined(__GNUC__) && (__GNUC__ >= 8) && !defined(__clang__)) || (defined(__clang_major__) && __clang_major__ == 8) I'm not sure if the #defined are necessary for __GNUC__ and __clang_major as on recent compilers the comparisons will just fail. So you might be able to simplify it like #if ((__GNUC__ >= 8) && !defined(__clang__)) || (__clang_major__ == 8) With the former you would probably want to split it up and have separate sections for the ||, possibly even separating out the #defined into separate lines as the current code does and duplicate the #define MAME_DEVCB_GNUC_BROKEN_FRIEND 1 With the later I would probably not split them up, but I can see there is an argument for it.

…

On 10/07/2020 20:43, Tiago wrote: @rb6502 <https://github.com/rb6502> Do you mean I should replicate the whole block for clang? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6933 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGVYYULN7ALQXYERKHZDDTR25VODANCNFSM4OVQVWQA>.

[ajrhacker]

I'm not sure if the #defined are necessary for __GNUC__ and __clang_major as on recent compilers the comparisons will just fail. So you might be able to simplify it like #if ((__GNUC__ >= 8) && !defined(__clang__)) || (__clang_major__ == 8)

Most popular compilers may pass that, but I don't believe that the C++ standard does. I think that clang at least warns about undefined values in #if expressions it with -Wall, and MAME builds with -Werror as well.

[smf-]

On 10/07/2020 22:29, ajrhacker wrote: Most popular compilers may pass that, but I don't believe that the C++ standard does. I think that clang at least warns about undefined values in #if expressions it with -Wall, and MAME builds with -Werror as well.

I tried the code with the msys2 versions of gcc 10.1.0 & clang 10.0.0 before sending the email and neither gives a warning with undefined #defines used like that. I haven't tested anything older, but you may well be right that they would complain. But then the original pull request is even more broken as it checks both variables if either of them is defined.

[rb6502]

I want it to check for Clang, then check the Clang version if it's Clang. If it's not check for GCC and do the existing GCC version check. That way there's no chance of checking undefined or faked compatibility version numbers and it's easier to read.

[tcamargo]

So, let's be verbose:

#if defined(__GNUC__)
#if (__GNUC__ >= 8)
#define MAME_DEVCB_GNUC_BROKEN_FRIEND 1
#endif // (__GNUC__ >= 8)
#endif // defined(__GNUC__)

#if defined(__clang__)
#if (__clang_major__ == 8)
#define MAME_DEVCB_GNUC_BROKEN_FRIEND 1
#endif // (__clang__ == 8)
#endif // defined(__clang__)

[rb6502]

I'd prefer it only do the GNUC check if it's not Clang, since Clang does set GNUC as @smf noted. But almost there :)

[smf-]

This still ignores that clang defines both __GNUC__ & __clang__, which is why the original has #ifdefined(__GNUC__) && !defined(__clang__)

…

On 12/07/2020 16:12, Tiago wrote: So, let's be verbose: |#if defined(__GNUC__) #if (__GNUC__ >= 8) #define MAME_DEVCB_GNUC_BROKEN_FRIEND 1 #endif // (__GNUC__ >= 8) #endif // defined(__GNUC__) #if defined(__clang__) #if (__clang_major__ == 8) #define MAME_DEVCB_GNUC_BROKEN_FRIEND 1 #endif // (__clang__ == 8) #endif // defined(__clang_major__) |

[tcamargo]

This still ignores that clang defines both GNUC & clang, which is why the original has #ifdefined(GNUC) && !defined(clang)
…
On 12/07/2020 16:12, Tiago wrote: So, let's be verbose: |#if defined(GNUC) #if (GNUC >= 8) #define MAME_DEVCB_GNUC_BROKEN_FRIEND 1 #endif // (GNUC >= 8) #endif // defined(GNUC) #if defined(clang) #if (clang_major == 8) #define MAME_DEVCB_GNUC_BROKEN_FRIEND 1 #endif // (clang == 8) #endif // defined(clang_major) |

Already corrected. ;) Thx for the feedback!