Digraph warning

[traits]

The original version can trigger a digraph-related warning in VS2015. Is this namespace construct a typo or something deliberate?

Files to change

There are currently two files which need to be edited:

1. src/json.hpp

2. test/src/unit.cpp - This contains the Catch unit tests which currently cover 100 % of the library's code.

   If you add or change a feature, please also add a unit test to this file. The unit tests can be compiled and executed with

   make check

   The test cases are also executed with several different compilers on Travis once you open a pull request.

Note

• If you open a pull request, the code will be automatically tested with Valgrind's Memcheck tool to detect memory leaks. Please be aware that the execution with Valgrind may in rare cases yield different behavior than running the code directly. This can result in failing unit tests which run successfully without Valgrind.

Please don't

• The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.8 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
• Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
• Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
• Please do not open pull requests that address multiple issues.

[theodelrieu]

I would prefer to insert a space here, but if astyle removes it, I don't see how this could cause any issues.

Except if one day there is a nlohmann namespace in std, but I doubt it :)

[gregmarr]

This token sequence is explicitly called out by the standard as not a digraph. What is the exact warning that you're getting?

Otherwise, if the next three characters are <:: and the subsequent character is neither : nor >, the <
is treated as a preprocessor token by itself and not as the first character of the alternative token <:.

[coveralls]

Coverage remained the same at 100.0% when pulling ab44a50 on traits:patch-1 into b32a6b8 on nlohmann:develop.

[traits]

What is the exact warning that you're getting?

sim01\json.hpp(14350): warning C4628: digraphs not supported with -Ze. Character sequence '<:' not interpreted as alternate token for '['

The project in question is a Visual Studio project, maintained by the proprietary UnrealEngine4 project/build system. This system seemingly turns on the VS2015 non-default -Ze switch during project creation.

Otherwise, if the next three characters are <:: and the subsequent character is neither : nor >, the <
is treated as a preprocessor token by itself and not as the first character of the alternative token <:.

So, this seems to be a VS C++11 incompatibility. This is classified as a non-issue during setting up the pull request. Slightly annoying, but eventually I have to accept this..

[gregmarr]

It also seems to turn on the C4628 warning, as it's off by default. I found that Qt dealt with this same compiler bug by adding a space after the <. If you can find a way to disable the C4628 warning, this should also take care of your issue.
It may be worthwhile both adding the space and reporting the compiler bug to Microsoft.

[gregmarr]

@nlohmann Two of the Travis builds failed because the compiler couldn't find the <sstream> header. Very strange.

[traits]

I found that Qt dealt with this same compiler bug by adding a space after the <.

Personally, I consider this fragile (bringing back to mind the good old days of ambiguous '<<') for similar reasons as mentioned by @theodelrieu. I have still to look-up the particular name-resolution rules - but perhaps the leading scope operator isn't necessary here? For the specialization immediately above (struct hash<nlohmann::json>) it isn't. Might differ for nlohmann::detail::value_t, of course.

[nlohmann]

@gregmarr I also noticed this, and I have no idea why Travis behaves this way. I think I need to remove these configurations.

[gregmarr]

I'm not sure what is more fragile, adding a space to eliminate a bogus compiler warning, or making the type specification less specific by removing the global specifier and allowing lookup in the local scope, with any using declarations that have been added. The latter can result in users not getting the behavior they expect. The former can result in the warning coming back if the space gets removed. Since the compiler warning can be silenced, or the compiler bug fixed, I'd lean towards adding the space. Note that I'm just a user, not a maintainer, so I don't make that decision, just provide input.

[nlohmann]

Removing the :: is not a good idea. Adding a space would be OK - astyle does not revert this change. Please make a comment regarding the necessity of the space with a link to this PR.

[traits]

Please make a comment regarding the necessity of the space with a link to this PR.

It seems, I am not quite familiar with some aspects of the github workflow. What do you mean here - a line comment on the patched line mentioning #679?

Topically - in the end it is of course your decision. I cannot say, that I know all possible code beautifiers, but astyle is certainly only one of them. clang for example has something much more detailed, I'm not sure if it could break these kind of things.

[nlohmann]

Yes, I meant a code comment like

// do not remove the space after '<', see https://github.com/nlohmann/json/pull/679

This project currently only uses astyle. I could not find a way to mimic the options I use in clang-format.

[nlohmann]

Thanks for the changes!

[coveralls]

Coverage remained the same at 100.0% when pulling 54c67dc on traits:patch-1 into b32a6b8 on nlohmann:develop.

[nlohmann]

Thanks!