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

Fix NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT with an empty JSON instance #4508

Merged
merged 4 commits into from
Dec 31, 2024

Conversation

NissimBendanan
Copy link
Contributor

@NissimBendanan NissimBendanan commented Nov 21, 2024

Do not throw an exception when trying to get default value from empty json
fix the issue #4507

class Foo {
 public:
  NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT(Foo, a, b)

  Foo() = default;

  int a{10};
  std::string b{"bar"};

  void log() {
    std::cout << "a: " << a << std::endl;
    std::cout << "b: " << b << std::endl;
  }
};

// with an empty json, an exception is thrown instead of getting default values
static void JsonTest_empty_json() {
  nlohmann::json json{};
  auto foo = Foo(json);
  foo.log();
}

Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.hpp. The whole process is described here.

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.7 (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 this kind of bug). 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.

include/nlohmann/json.hpp Outdated Show resolved Hide resolved
@nlohmann nlohmann linked an issue Nov 21, 2024 that may be closed by this pull request
2 tasks
@nlohmann

This comment was marked as outdated.

@NissimBendanan
Copy link
Contributor Author

NissimBendanan commented Dec 3, 2024

@gregmarr I will update the PR according to our decision to change only this macro NLOHMANN_JSON_FROM_WITH_DEFAULT

I will change only this file: include\nlohmann\detail\macro_scope.hpp
I will not touch this one: single_include\nlohmann\json.hpp
right?

should also add a test case or update a previous one?
any clue to do this? in which file? how to compile/run the tests?

Thanks

@github-actions github-actions bot added S and removed M labels Dec 3, 2024
@gregmarr
Copy link
Contributor

gregmarr commented Dec 3, 2024

@NissimBendanan
From the checklist in the PR template:

The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.hpp. The whole process is described here.

@gregmarr
Copy link
Contributor

gregmarr commented Dec 3, 2024

NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT is tested in https://github.com/nlohmann/json/blob/develop/tests/src/unit-udt_macro.cpp

…TRUSIVE_WITH_DEFAULT work with an empty JSON instance
@NissimBendanan
Copy link
Contributor Author

I pushed a new version where I added a check of an empty json.
@nlohmann @gregmarr can you please review?
Thanks

Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @NissimBendanan
Please read and follow the Contribution Guidelines.

@coveralls
Copy link

coveralls commented Dec 26, 2024

Coverage Status

coverage: 99.639%. remained the same
when pulling 548c47b on NissimBendanan:fix_default_value
into 6057b31 on nlohmann:develop.

@NissimBendanan
Copy link
Contributor Author

I don't understand how to get rid of those errors. here in Amalgamation check failed! 🔴

Format (1)
2754c2754
< #define NLOHMANN_JSON_FROM_WITH_DEFAULT(v1) nlohmann_json_t.v1 = nlohmann_json_j.value(#v1, nlohmann_json_default_obj.v1);

#define NLOHMANN_JSON_FROM_WITH_DEFAULT(v1) nlohmann_json_t.v1 = !nlohmann_json_j.is_null() ? nlohmann_json_j.value(#v1, nlohmann_json_default_obj.v1) : nlohmann_json_default_obj.v1;
Error: Process completed with exit code 1.

locally the single_include files are generated with success.
so so how to fix all of those errors?

@nlohmann
Copy link
Owner

Use Astyle 3.1 and run make amalgamate.

@NissimBendanan
Copy link
Contributor Author

I am using astyle 3.1:

nbendana@tlv-wp4ww:/Projects/json$ astyle --version
Artistic Style Version 3.1
nbendana@tlv-wp4ww:
/Projects/json$ make amalgamate
make pretty
make[1]: Entering directory '/home/nbendana/Projects/json'
astyle
--style=allman
--indent=spaces=4
--indent-modifiers
--indent-switches
--indent-preproc-block
--indent-preproc-define
--indent-col1-comments
--pad-oper
--pad-header
--align-pointer=type
--align-reference=type
--add-braces
--convert-tabs
--close-templates
--lineend=linux
--preserve-date
--suffix=none
--formatted
include/nlohmann/adl_serializer.hpp include/nlohmann/byte_container_with_subtype.hpp include/nlohmann/detail/abi_macros.hpp include/nlohmann/detail/conversions/from_json.hpp include/nlohmann/detail/conversions/to_chars.hpp include/nlohmann/detail/conversions/to_json.hpp include/nlohmann/detail/exceptions.hpp include/nlohmann/detail/hash.hpp include/nlohmann/detail/input/binary_reader.hpp include/nlohmann/detail/input/input_adapters.hpp include/nlohmann/detail/input/json_sax.hpp include/nlohmann/detail/input/lexer.hpp include/nlohmann/detail/input/parser.hpp include/nlohmann/detail/input/position_t.hpp include/nlohmann/detail/iterators/internal_iterator.hpp include/nlohmann/detail/iterators/iter_impl.hpp include/nlohmann/detail/iterators/iteration_proxy.hpp include/nlohmann/detail/iterators/iterator_traits.hpp include/nlohmann/detail/iterators/json_reverse_iterator.hpp include/nlohmann/detail/iterators/primitive_iterator.hpp include/nlohmann/detail/json_custom_base_class.hpp include/nlohmann/detail/json_pointer.hpp include/nlohmann/detail/json_ref.hpp include/nlohmann/detail/macro_scope.hpp include/nlohmann/detail/macro_unscope.hpp include/nlohmann/detail/meta/call_std/begin.hpp include/nlohmann/detail/meta/call_std/end.hpp include/nlohmann/detail/meta/cpp_future.hpp include/nlohmann/detail/meta/detected.hpp include/nlohmann/detail/meta/identity_tag.hpp include/nlohmann/detail/meta/is_sax.hpp include/nlohmann/detail/meta/std_fs.hpp include/nlohmann/detail/meta/type_traits.hpp include/nlohmann/detail/meta/void_t.hpp include/nlohmann/detail/output/binary_writer.hpp include/nlohmann/detail/output/output_adapters.hpp include/nlohmann/detail/output/serializer.hpp include/nlohmann/detail/string_concat.hpp include/nlohmann/detail/string_escape.hpp include/nlohmann/detail/value_t.hpp include/nlohmann/json.hpp include/nlohmann/json_fwd.hpp include/nlohmann/ordered_map.hpp include/nlohmann/thirdparty/hedley/hedley.hpp include/nlohmann/thirdparty/hedley/hedley_undef.hpp tests/abi/config/config.hpp tests/abi/config/custom.cpp tests/abi/config/default.cpp tests/abi/config/noversion.cpp tests/abi/diag/diag.cpp tests/abi/diag/diag.hpp tests/abi/diag/diag_off.cpp tests/abi/diag/diag_on.cpp tests/abi/inline_ns/use_current.cpp tests/abi/inline_ns/use_v3_10_5.cpp tests/abi/main.cpp tests/benchmarks/src/benchmarks.cpp tests/cmake_add_subdirectory/project/main.cpp tests/cmake_fetch_content/project/main.cpp tests/cmake_fetch_content2/project/main.cpp tests/cmake_import/project/main.cpp tests/cmake_import_minver/project/main.cpp tests/cmake_target_include_directories/project/Bar.cpp tests/cmake_target_include_directories/project/Bar.hpp tests/cmake_target_include_directories/project/Foo.cpp tests/cmake_target_include_directories/project/Foo.hpp tests/cmake_target_include_directories/project/main.cpp tests/cuda_example/json_cuda.cu tests/src/fuzzer-driver_afl.cpp tests/src/fuzzer-parse_bjdata.cpp tests/src/fuzzer-parse_bson.cpp tests/src/fuzzer-parse_cbor.cpp tests/src/fuzzer-parse_json.cpp tests/src/fuzzer-parse_msgpack.cpp tests/src/fuzzer-parse_ubjson.cpp tests/src/make_test_data_available.hpp tests/src/test_utils.hpp tests/src/unit-32bit.cpp tests/src/unit-algorithms.cpp tests/src/unit-allocator.cpp tests/src/unit-alt-string.cpp tests/src/unit-assert_macro.cpp tests/src/unit-binary_formats.cpp tests/src/unit-bjdata.cpp tests/src/unit-bson.cpp tests/src/unit-byte_container_with_subtype.cpp tests/src/unit-capacity.cpp tests/src/unit-cbor.cpp tests/src/unit-class_const_iterator.cpp tests/src/unit-class_iterator.cpp tests/src/unit-class_lexer.cpp tests/src/unit-class_parser.cpp tests/src/unit-comparison.cpp tests/src/unit-concepts.cpp tests/src/unit-constructor1.cpp tests/src/unit-constructor2.cpp tests/src/unit-convenience.cpp tests/src/unit-conversions.cpp tests/src/unit-custom-base-class.cpp tests/src/unit-deserialization.cpp tests/src/unit-diagnostics.cpp tests/src/unit-disabled_exceptions.cpp tests/src/unit-element_access1.cpp tests/src/unit-element_access2.cpp tests/src/unit-hash.cpp tests/src/unit-inspection.cpp tests/src/unit-items.cpp tests/src/unit-iterators1.cpp tests/src/unit-iterators2.cpp tests/src/unit-json_patch.cpp tests/src/unit-json_pointer.cpp tests/src/unit-large_json.cpp tests/src/unit-merge_patch.cpp tests/src/unit-meta.cpp tests/src/unit-modifiers.cpp tests/src/unit-msgpack.cpp tests/src/unit-no-mem-leak-on-adl-serialize.cpp tests/src/unit-noexcept.cpp tests/src/unit-ordered_json.cpp tests/src/unit-ordered_map.cpp tests/src/unit-pointer_access.cpp tests/src/unit-readme.cpp tests/src/unit-reference_access.cpp tests/src/unit-regression1.cpp tests/src/unit-regression2.cpp tests/src/unit-serialization.cpp tests/src/unit-testsuites.cpp tests/src/unit-to_chars.cpp tests/src/unit-type_traits.cpp tests/src/unit-ubjson.cpp tests/src/unit-udl.cpp tests/src/unit-udt.cpp tests/src/unit-udt_macro.cpp tests/src/unit-unicode1.cpp tests/src/unit-unicode2.cpp tests/src/unit-unicode3.cpp tests/src/unit-unicode4.cpp tests/src/unit-unicode5.cpp tests/src/unit-user_defined_input.cpp tests/src/unit-windows_h.cpp tests/src/unit-wstring.cpp tests/src/unit.cpp single_include/nlohmann/json.hpp single_include/nlohmann/json_fwd.hpp docs/examples/*.cpp
make[1]: Leaving directory '/home/nbendana/Projects/json'
nbendana@tlv-wp4ww:~/Projects/json$

@NissimBendanan
Copy link
Contributor Author

for this failure:

ci_static_analysis (ci_test_amalgamation)

I run locally without error:

nbendana@tlv-wp4ww:~/Projects/json$ cmake -S . -B build -DJSON_CI=On
-- Found Python3: /usr/bin/python3 (found version "3.12.3") found components: Interpreter 
-- 🔖 Artistic Style 3.1 (/usr/bin/astyle)
-- 🔖 Clang 18.1.3 (/usr/bin/clang++)
-- 🔖 Clang-Tidy 18.1.3 (/usr/bin/clang-tidy)
-- 🔖 CMake 3.28.3 (/usr/bin/cmake)
-- 🔖 Cppcheck  (CPPCHECK_TOOL-NOTFOUND)
-- 🔖 GCC 13.3.0 (/usr/bin/g++-13)
-- 🔖 GCOV 13.3.0 (/usr/bin/gcov)
-- 🔖 Git 2.43.0 (/usr/bin/git)
-- 🔖 include-what-you-use  (IWYU_TOOL-NOTFOUND)
-- 🔖 Infer  (INFER_TOOL-NOTFOUND)
-- 🔖 LCOV  (LCOV_TOOL-NOTFOUND)
-- 🔖 Ninja 1.11.1 (/usr/bin/ninja)
-- 🔖 OCLint  (OCLINT_TOOL-NOTFOUND)
-- 🔖 Valgrind  (VALGRIND_TOOL-NOTFOUND)
-- Using the multi-header code from /home/nbendana/Projects/json/include/
-- Operating system: Linux-5.15.167.4-microsoft-standard-WSL2; Distributor ID:  Ubuntu; Description:    Ubuntu 24.04.1 LTS; Release:    24.04; Codename:        noble; Linux tlv-wp4ww 5.15.167.4-microsoft-standard-WSL2 #1 SMP Tue Nov 5 00:21:55 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
-- Compiler: c++ (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0; Copyright (C) 2023 Free Software Foundation, Inc.; This is free software; see the source for copying conditions.  There is NO; warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
-- Testing standards: 11 14 17 20 23
-- Configuring done (3.7s)
-- Generating done (0.3s)
-- Build files have been written to: /home/nbendana/Projects/json/build
nbendana@tlv-wp4ww:~/Projects/json$ cmake --build build --target ci_test_amalgamation
Check amalgamation and indentation
Creating amalgamation:
 - processing "include/nlohmann/json.hpp"
...done!


Creating amalgamation:
 - processing "include/nlohmann/json_fwd.hpp"
...done!


Built target ci_test_amalgamation
nbendana@tlv-wp4ww:~/Projects/json$ 

Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @NissimBendanan
Please read and follow the Contribution Guidelines.

@nlohmann
Copy link
Owner

I am working on #4573 to fix the astyle call.

@nlohmann
Copy link
Owner

#4573 is merged now. Please update to the latest develop branch and try again.

@NissimBendanan
Copy link
Contributor Author

NissimBendanan commented Dec 30, 2024

after updating with last version, I still have this error:

Formatted  /home/runner/work/json/json/single_include/nlohmann/json.hpp
2764c2764
< #define NLOHMANN_JSON_FROM_WITH_DEFAULT(v1) nlohmann_json_t.v1 = nlohmann_json_j.value(#v1, nlohmann_json_default_obj.v1);
---
> #define NLOHMANN_JSON_FROM_WITH_DEFAULT(v1) nlohmann_json_t.v1 = !nlohmann_json_j.is_null() ? nlohmann_json_j.value(#v1, nlohmann_json_default_obj.v1) : nlohmann_json_default_obj.v1;
gmake[3]: *** [CMakeFiles/ci_test_amalgamation.dir/build.make:80: CMakeFiles/ci_test_amalgamation] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1484: CMakeFiles/ci_test_amalgamation.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1491: CMakeFiles/ci_test_amalgamation.dir/rule] Error 2
gmake: *** [Makefile:511: ci_test_amalgamation] Error 2

Can you please explain what does this error mean? which check generate this error? how to get rid of it?
Thanks

@nlohmann
Copy link
Owner

You only changed the code in the include folder. From this, the single-header sources in single_include need to be created. This is done by calling make amalgamate. Please execute this step and commit the changes.

Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @NissimBendanan
Please read and follow the Contribution Guidelines.

@NissimBendanan
Copy link
Contributor Author

so I need also to commit the change in single_include/nlohmann/json.hpp ?
From CONTRIBUTING.md it is not clear that I need to commit the changes of this file, just that it is auto generated ...

…TRUSIVE_WITH_DEFAULT work with an empty JSON instance
@nlohmann
Copy link
Owner

The contribution guidelines state:

Please do not edit the files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.hpp directly, but change the include/nlohmann sources and regenerate the files by executing make amalgamate.

I am currently revising the guidelines to make this more clear.

@github-actions github-actions bot added M and removed S labels Dec 30, 2024
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann added this to the Release 3.11.4 milestone Dec 31, 2024
@nlohmann nlohmann merged commit 2134cb9 into nlohmann:develop Dec 31, 2024
122 checks passed
@nlohmann
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT throw an exception with an empty json
4 participants