C++ defined feature checks in base.h

[KageKirin]

Hello,

For the context, I ran into this issue while figuring out why my changes from #4640 don't give me constexpr hash functions on Mac (Xcode9).

There are many C++ feature checks in base.h that check for _MSC_VER or __GNUC__ but omit __clang__, hence resolving to false when compiling with clang (especially Xcode-clang).

I propose the following solutions:

• adding checks for __clang__
• replacing said checks with checks for the C++ version, e.g. defined(__cplusplus) && __cplusplus >= 201103L for C++11
• replacing said checks with checks for the required C++ feature, e.g. defined(__cpp_constexpr) && __cpp_constexpr >= 200704 for C++11 N2235 "Generalized Constant Expressions"

The different macros are listed here. Here are the lists of the respective implementation status by Clang and GCC.

If I were to make the changes, I would opt for options 2 and 3, i.e. check for __cplusplus version and for __cpp_feature, as I did in #4640, (i.e., I already have a WIP patch for this), but I'm open for other opinions, especially regarding backwards compatibility with older or less-ISO-C++-compliant compilers.

Regards.

[aardappel]

I'm definitely in favor of improving the feature detection in general.

Not sure if just going by the official defines will work though, as e.g. for Visual Studio __cplusplus >= 201103L may be false for quite a few versions even though the feature in question is available. We can't regress in what we support.

Also problematic is our support of STLPort, where the compiler may be very new but none of the STL features will be available.

[sssilver]

This is a major show-stopper for us as we need to build our project for many different platforms, including macOS/iOS, and use clang extensively even outside of Apple's platforms.

We'd be happy to submit a PR that fixes this, however it's tricky to ensure that the change doesn't break otherwise sane checks on compilers/platforms that misbehave, as you've pointed out.

What would be the optimal way forward?

[aardappel]

I actually had to disable the hash functions being constexpr at all recently (it is on master) as it broke things internally for us as well: https://github.com/google/flatbuffers/commit/ca68d8b0433e39d788aefcd4c01a93cadcdc43f4 . It would definitely need better feature detection to be turned back on, and work with -Wc++98-c++11-compat.

I welcome any PRs that can improve the situation.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity for 1 year. It will be automatically closed if no further activity occurs. To keep it open, simply post a new comment. Maintainers will re-open on new activity. Thank you for your contributions.