Add support for workarounds for non-compliant C++11 compilers

[TedLyngmo]

Include example for Borland C++ Builder (RAD Studio)'s clang (3.3.1) based compiler (bcc32c).

I know this goes against the Contribution Guidelines and I will keep my fork rebased separately if this is unacceptable. I would nevertheless like your input please.

What I've done is to add a macro (NLOHMANN_JSON_SKIP_COMPILER_CHECK) to be able override the compiler check and one macro (NLOHMANN_JSON_FIX_CONSTEXPR_FOR_CHAR_TRAITS_EOF) to let a porter define a constexpr instead of using std::char_traits::eof(). I couldn't find a way to override the built in definition unfortunately. If you know of a way that this could be done, I'd be glad to use that way instead.

The file "include/nlohmann/detail/workarounds/borland.hpp" will not be included in the amalgamated json.hpp - so any user will have to manually include it before json.hpp. I'll happily remove this file if the other changes would be acceptable.

[coveralls]

Coverage remained the same at 100.0% when pulling 5e0c024 on TedLyngmo:borland_port into e830bc5 on nlohmann:develop.

[OvermindDL1]

A curiosity thing, what is special about that compiler? What does it do that is unique? Why would someone use it over a modern standard compiler?

[TedLyngmo]

@OvermindDL1 That's a fair question :-) It's not the compiler as such that makes it unique but the IDE i.m.o. I've used it (and its predecessors) for the past ~18 years - but I really did expect better support for C++11 when I bought this release. I also didn't like the built-in JSON support so I went for this JSON lib that I've used and liked before.

[OvermindDL1]

@TedLyngmo Ah, what's special about the IDE then? I use KDevelop myself (CLion is interesting as well but it is soooooo slooooow compared to KDevelop) and find it absolutely fantastic (plus it was one of the first to use CMake as it's main build system, one of the first (if not 'the' first) to use libclang for intellisense, etc... etc...), but I'm always curious in something better.

[TedLyngmo]

Well, when I first started using it, the closest competitor was Visual Studio - and that was a real pain at the time. C++ Builder haven't changed that much since then so I guess many other IDE:s have caught up. You can check it out here if you are curious to know more: C++ Builder. Anyway, there's not that much missing it it for me to be able to overcome it - and having the possibility to disable the version check (and also by being able to supply an alternative constexpr for eof()) would really help.

[OvermindDL1]

That... is a very odd IDE... a custom GUI framework that doesn't match modern things, doesn't run on linux, has this weird firemonkey thing all of it (which further seems to be a proprietary thing that only runs in specific areas), among more... I'm not sure how I could even use that (linux user) first of all, second it doesn't seem to support modern standards, even the (very little) documentation I could find on cmake encourages using the very old command invocations, as well as it doesn't seem to support cmake projects at all other than building them (where if you want to edit them then you have to essentially re-write it into their ecosystem, which does not seem easily possible considering how much I have cmake generate code for me...)... o.O

Plus a HolyCrapTonOfMoney, who'd pay that for just an IDE especially when its feature listings don't imply anything worth that at all?! O.o

Interesting read though, didn't know such an odd ecosystem existed, that's very odd... ^.^;

[TedLyngmo]

Clearly not the IDE for you :-) Oh well, any comments on the commit as such? I'd llike the input even if it's utterly rejected :)

[OvermindDL1]

Well to me it looks slightly less portable, though not in any way on any architecture that someone using this library would be using, so otherwise it looks fine to me and I'd probably accept it after the usual bout of testing. It's all up to owner though. ^.^

[theodelrieu]

Symbols beginning with underscore followed by a capital letter are reserved by the standard.
You should remove the first and last underscores to be compliant.

(Applies to other macros added in this PR)

[TedLyngmo]

Thanks, fixed that in the new commit.

[theodelrieu]

Adding definitions to std being forbidden by the standard, I guess the chances of this PR being merged are quite low...

But you know that already :p.

[TedLyngmo]

Sure did. :-) I removed the workaround file in the new commit.

[TedLyngmo]

Less portable is quite the opposite of what I was trying to achieve :-)

Are you thinking about the actual workaround/borland.hpp file?

I'm thinking of removing that file from this change and put it (without the actual defines) in a repo of its own so that anyone (with the specific platform it works on) in need of the missing cmath functions could use it in any project - and combining that file with the two defines, using this json lib will work fine.

Perhaps that's a cleaner approach?

[theodelrieu]

It does look better without the workaround file. But it might open pandora's box to introduce such a customization point (the FIX_FOR_CONSTEXPR_EOF stuff).

I feat it will create a precedent, and some people using another non-compliant compiler might have different workarounds for different problems.

If tomorrow you find two or three more quirks inside Borland, which also require such a macro definition in upstream, that will be problematic.

Furthermore, since there is no CI at the moment for this compiler, we have no way to know if future code will break those.

Maybe the best solution will be to fork every release, add your patch, and make a release for JSON for Modern Borland C++?

[TedLyngmo]

I agree that the CONSTEXPR macro could be dangerous. I removed it so now there's only the NLOHMANN_JSON_SKIP_COMPILER_CHECK macro left.

I kept this (now unconditional):
static constexpr std::char_traits<char>::int_type end_of_file = std::char_traits<char>::eof();

It'll make patching easier and i.m.o. is easier to read when used.

[theodelrieu]

Sounds reasonable to me, I have no more issues with your PR :)

[nlohmann]

I don't really like this extension. We support C++11-compliant compilers, and how could we justify a line like

static constexpr std::char_traits<char>::int_type end_of_file = std::char_traits<char>::eof();

if std::char_traits::eof is defined by C++11 to be static constexpr? We would need to annotate the definition above with comments like "do not remove - some compiler that is not part of our CI needs this line!". Sorry, but I fear that this begs for more workarounds in the future which would require more effort in keeping track which parts of the code must not be "modernized".

We may, however, thing about guarding the compiler check with #ifndef NDEBUG - what do you think?

[TedLyngmo]

About the static constexpr: The way I see it, it's just another convenient constant and I don't think there's a need for an annotation like that. If the line is removed, the checks won't compile. One would have to replace all end_of_file's with std::char_traits<char>::eof() too. That can surely happen, but I don't see much risk with it, should it ever happen, I was contemplating going for auto instead, should the std::char_traits::eof definition change in some later C++ version:

static constexpr auto end_of_file = std::char_traits<char>::eof();

About the compiler check: If I understand the #ifndef NDEBUG thought correctly, the compiler check would then be performed whenever someone is debug compiling, but not when compiling for release? That could perhaps cause some confusion for someone starting directly with release compilation, who would then be getting strange errors instead of the "unsupported compiler version" message. I would prefer an option that must be deliberately set and makes it clear what it does (and also makes it possible to still debug compile).

[nlohmann]

The thing with the constant end_of_file is that it is not a matter of convenience (meaning I could use it instead of calling std::char_traits<char>::eof()), but rather a matter whether or not your IDE can work. Therefore, we would need an annotation, because making the equivalent change by replacing all end_of_file occurrences with std::char_traits<char>::eof() is not equivalent for your IDE. We would also need to make sure to never use std::char_traits<char>::eof() in the future in other parts of the code.

About NDEBUG - you are right: it may be confusing if code breaks in debug mode but works in production mode.

[TedLyngmo]

My IDE won't work even with the end_of_file constant declared like in my PR. It won't compile since it has the old non-constexpr definition of std::char_traits::eof, so replacing all end_of_file occurrences with std::char_traits<char>::eof() would result in the same error. I still need to patch the end_of_file line to make it work.

#ifdef NLOHMANN_JSON_EOF_CONSTEXPR_FIX
  static constexpr std::char_traits<char>::int_type end_of_file = NLOHMANN_JSON_EOF_CONSTEXPR_FIX;
#else
  static constexpr std::char_traits<char>::int_type end_of_file = std::char_traits<char>::eof();
#endif

but I'd keep that patch in my fork only (unless the above would be acceptable, which I'm not expecting).

For compliant compilers, the two (end_of_file or std::char_traits<char>::eof()) could be used interchangeably. Only those (me) who's gone out on a limb and defining NLOHMANN_JSON_SKIP_COMPILER_CHECK would notice - and if that happens after I rebase, I'll submit a new PR where end_of_file is used instead :-)

[nlohmann]

Then I misunderstood, sorry. But adding more macros to mitigate noncompliant compilers is even worse. Sorry, but I cannot accept this PR.

[TedLyngmo]

I get that, but just to be clear: The NLOHMANN_JSON_EOF_CONSTEXPR_FIX macro isn't part of the PR. I keep that macro in my fork.

The only macro added in the PR is NLOHMANN_JSON_SKIP_COMPILER_CHECK. Would you reconsider a PR with only that macro in it? It could be renamed into something like NLOHMANN_JSON_USE_UNSUPPORTED_COMPILER_AND_EXPECT_NO_SUPPORT to emphasize that using it means flying solo.

[nlohmann]

You mean guarding the compiler check would be sufficient?

[TedLyngmo]

Yes, I think I can make it work with that.

[nlohmann]

You can now skip the compiler check by defining JSON_SKIP_UNSUPPORTED_COMPILER_CHECK.

[TedLyngmo]

Thanks! :-)

[nlohmann]

You're welcome. Sorry for the long discussion.

[TedLyngmo]

No worries, I get it. I've been filing bug reports to the compiler vendor too but have little hope of getting an update without additional fees. Also started making adaptions to their STL implementation but it's a real mess so I don't think I'll have much luck with that. I've been able to add their missing cmath functions though, so that's working anyway, but this is very frustrating...