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

Patch to fix Issue #230 #240

Closed
wants to merge 13 commits into from
Closed

Conversation

camerondruyor
Copy link

Files to change

There are currently two files which need to be edited:

  1. src/json.hpp.re2c (note the .re2c suffix) - This file contains a comment section which describes the JSON lexic. This section is translated by re2c into file src/json.hpp which is plain "vanilla" C++11 code. (In fact, the generated lexer consists of some hundred lines of gotos, which is a hint you never want to edit this file...).

    If you only edit file src/json.hpp (without the .re2c suffix), your changes will be overwritten as soon as the lexer is touched again. To generate the src/json.hpp file which is actually used during compilation of the tests and all other code, please execute

    make re2c

    To run re2c and generate/overwrite file src/json.hpp with your changes in file src/json.hpp.re2c.

  2. test/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 with

    make

    and can be executed with

    ./json_unit

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

Please understand that I cannot accept pull requests changing only file src/json.hpp.

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

  • Only make changes to file src/json.hpp -- please read the paragraph above and understand why src/json.hpp.re2c exists.
  • 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.

@camerondruyor
Copy link
Author

Oops, I didn't realize how the hpp file was generated. I will push my changes into re2c instead.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 99.75% when pulling faa5a4d on Kamtron:develop into 74544b4 on nlohmann:develop.

@nlohmann
Copy link
Owner

No worries. If you have a hint how to improve the documentation for committers, please let me know!

Looking forward to your fix!

…made my changes to json.hpp.re2c as I should have.

Tests back to green, with no direct edits to json.hpp
@camerondruyor
Copy link
Author

Hmm, Travis build is failing because it looks like the build machine can't find cmake.

The builds on my machine and AppVeyor seem ok, though.

Any ideas @nlohmann ? (sorry for being a pain!)

@nlohmann
Copy link
Owner

The reason for the error is that you overwrote the project's Makefile locally and committed this change. Could you please restore the Makefile or just exclude it from your commit?

@@ -7848,9 +7848,8 @@ class basic_json
result.m_type.bits.parsed = true;

// 'found_radix_point' will be set to 0xFF upon finding a radix
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment needs to be adjusted to the fact that found_radix_point is a Boolean now ("set to 0xFF" makes no sense).

@nlohmann
Copy link
Owner

@KAMTRON , I added some notes to the changes of json.hpp.re2c and unit.cpp.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 99.739% when pulling 372c6dd on Kamtron:develop into 74544b4 on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.008%) to 99.791% when pulling fbcf150 on Kamtron:develop into 74544b4 on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 99.843% when pulling 2e4d493 on Kamtron:develop into 74544b4 on nlohmann:develop.

@nlohmann
Copy link
Owner

The roundtrip feature is still buggy and will not make it into 2.0.0. Therefore, I close this PR. Thanks so much @KAMTRON for your work. I hope you understand that the this PR does not fix the problem of having "100%" correct roundtrip behavior.

See #230 (comment) for the discussion.

@nlohmann nlohmann closed this Jun 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants