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

Add C++17 copies of the test binaries #3101

Merged
merged 40 commits into from
Dec 29, 2021
Merged

Add C++17 copies of the test binaries #3101

merged 40 commits into from
Dec 29, 2021

Conversation

nlohmann
Copy link
Owner

This PR makes sure C++17 is used additionally where the test suite is using C++17 features.

Fixes #3090.

@nlohmann nlohmann self-assigned this Oct 20, 2021
@nlohmann nlohmann marked this pull request as draft October 20, 2021 10:20
@coveralls
Copy link

coveralls commented Oct 20, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling eba77cd on issue3090 into 825d323 on develop.

@nlohmann nlohmann added the state: help needed the issue needs help to proceed label Oct 20, 2021
@nlohmann
Copy link
Owner Author

For some reason, the test with Clang9 on Ubuntu is currently failing with a segmentation fault. It would be great if anyone had an idea.

@theodelrieu
Copy link
Contributor

I wonder if we should bother supporting non-conforming C++17 compilers?

Or at least we could rely on __has_include to check for filesystem then experimental/filesystem.

@gregmarr
Copy link
Contributor

Or at least we could rely on __has_include to check for filesystem then experimental/filesystem.

That's what this PR is doing, after the feature test macros, which are supposed to be better indicators.

@theodelrieu
Copy link
Contributor

@nlohmann Looks like it still chooses the CompatibleArrayType overload on that Ubuntu-clang9. I don't have such a setup near me unfortunately.

@nlohmann
Copy link
Owner Author

You can run that configuration in Docker with

docker run -it ghcr.io/nlohmann/json-ci:v1.0.0 /bin/bash
git pull https://github.com/nlohmann/json.git
cd json
git checkout issue3090
cmake -S . -B build -DJSON_CI=On
cmake --build build --target ci_test_compiler_clang++-9

Running it individually, I get:

===============================================================================
/root/json/test/src/unit-regression2.cpp:302:
TEST CASE:  regression tests 2
  issue #3070 - Version 3.10.3 breaks backward-compatibility with 3.10.2 

/root/json/test/src/unit-regression2.cpp:808: SUCCESS: CHECK( j_path == text_path ) is correct!
  values: CHECK( "/tmp/text.txt" == "/tmp/text.txt" )

/root/json/test/src/unit-regression2.cpp:810: SUCCESS: CHECK_THROWS_WITH_AS( nlohmann::detail::std_fs::path(json(1)), "[json.exception.type_error.302] type must be string, but is number", json::type_error ) threw as expected!

/root/json/test/src/unit-regression2.cpp:302: FATAL ERROR: test case CRASHED: SIGSEGV - Segmentation violation signal
```�

When running in gdb, I get this stacktrace:

Program received signal SIGSEGV, Segmentation fault.
0x00007f0ff5adecb0 in std::filesystem::__cxx11::path::_List::_Impl_deleter::operator()(std::filesystem::__cxx11::path::_List::_Impl*) const ()
from /lib/x86_64-linux-gnu/libstdc++.so.6
(gdb) bt
#0 0x00007f0ff5adecb0 in std::filesystem::__cxx11::path::_List::_Impl_deleter::operator()(std::filesystem::__cxx11::path::_List::_Impl*) const ()
from /lib/x86_64-linux-gnu/libstdc++.so.6
#1 0x0000000000687a1d in std::unique_ptr<std::filesystem::__cxx11::path::_List::_Impl, std::filesystem::__cxx11::path::_List::_Impl_deleter>::unique_ptr
(this=0x7ffe27733d60) at /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/unique_ptr.h:361
#2 0x0000000000687955 in std::filesystem::__cxx11::path::_List::
_List (this=0x7ffe27733d60)
at /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/fs_path.h:649
#3 0x00000000005ab2b6 in std::filesystem::__cxx11::path::~path (this=0x7ffe27733d40)
at /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/fs_path.h:310
#4 0x000000000059dd24 in _DOCTEST_ANON_FUNC_2 () at /root/json/test/src/unit-regression2.cpp:811
#5 0x0000000000572797 in doctest::Context::run (this=0x7ffe27737b38) at /root/json/test/thirdparty/doctest/doctest.h:6486
#6 0x0000000000573816 in main (argc=2, argv=0x7ffe27737c48) at /root/json/test/thirdparty/doctest/doctest.h:6571

@nlohmann nlohmann added release item: 🐛 bug fix and removed state: help needed the issue needs help to proceed labels Dec 28, 2021
@nlohmann nlohmann added this to the Release 3.10.5 milestone Dec 28, 2021
@nlohmann nlohmann marked this pull request as ready for review December 28, 2021 21:48
@nlohmann nlohmann merged commit 6d31159 into develop Dec 29, 2021
@nlohmann nlohmann deleted the issue3090 branch December 29, 2021 08:47
@YarikTH
Copy link
Contributor

YarikTH commented Dec 29, 2021

#if !defined(JSON_HAS_FILESYSTEM) && !defined(JSON_HAS_EXPERIMENTAL_FILESYSTEM)
    #ifdef JSON_HAS_CPP_17
        #if defined(__cpp_lib_filesystem)
            #define JSON_HAS_FILESYSTEM 1
        #elif defined(__cpp_lib_experimental_filesystem)
            #define JSON_HAS_EXPERIMENTAL_FILESYSTEM 1
        #elif !defined(__has_include)
            #define JSON_HAS_EXPERIMENTAL_FILESYSTEM 1
        #elif __has_include(<filesystem>)
            #define JSON_HAS_FILESYSTEM 1
        #elif __has_include(<experimental/filesystem>)
            #define JSON_HAS_EXPERIMENTAL_FILESYSTEM 1
        #endif

        // std::filesystem does not work on MinGW GCC 8: https://sourceforge.net/p/mingw-w64/bugs/737/
        #if defined(__MINGW32__) && defined(__GNUC__) && __GNUC__ == 8
            #undef JSON_HAS_FILESYSTEM
            #undef JSON_HAS_EXPERIMENTAL_FILESYSTEM
        #endif

        // no filesystem support before GCC 8: https://en.cppreference.com/w/cpp/compiler_support
        #if defined(__GNUC__) && !defined(__clang__) && __GNUC__ < 8
            #undef JSON_HAS_FILESYSTEM
            #undef JSON_HAS_EXPERIMENTAL_FILESYSTEM
        #endif

        // no filesystem support before Clang 7: https://en.cppreference.com/w/cpp/compiler_support
        #if defined(__clang_major__) && __clang_major__ < 7
            #undef JSON_HAS_FILESYSTEM
            #undef JSON_HAS_EXPERIMENTAL_FILESYSTEM
        #endif

        // no filesystem support before MSVC 19.14: https://en.cppreference.com/w/cpp/compiler_support
        #if defined(_MSC_VER) && _MSC_VER < 1940
            #undef JSON_HAS_FILESYSTEM
            #undef JSON_HAS_EXPERIMENTAL_FILESYSTEM
        #endif

        // no filesystem support before iOS 13
        #if defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130000
            #undef JSON_HAS_FILESYSTEM
            #undef JSON_HAS_EXPERIMENTAL_FILESYSTEM
        #endif

        // no filesystem support before macOS Catalina
        #if defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
            #undef JSON_HAS_FILESYSTEM
            #undef JSON_HAS_EXPERIMENTAL_FILESYSTEM
        #endif
    #endif
#endif

#ifndef JSON_HAS_EXPERIMENTAL_FILESYSTEM
    #define JSON_HAS_EXPERIMENTAL_FILESYSTEM 0
#endif

#ifndef JSON_HAS_FILESYSTEM
    #define JSON_HAS_FILESYSTEM 0
#endif

Why do you combine has_include checks with version range checks? As a result, for example on GCC7, there is a more or less working experimental filesystem and has_include should detect this, but in the version range check you just erase results for all has_include checks just because some outdated table from the internet says so. Looks strange.

It doesn't bother my projects, I don't pass filesystem path via json and I have defines to override the behaviour, so for me it is still fine.

Also duplicating block worth its own header in details.

#if JSON_HAS_EXPERIMENTAL_FILESYSTEM
#include <experimental/filesystem>
namespace nlohmann::detail
{
namespace std_fs = std::experimental::filesystem;
} // namespace nlohmann::detail
#elif JSON_HAS_FILESYSTEM
#include <filesystem>
namespace nlohmann::detail
{
namespace std_fs = std::filesystem;
} // namespace nlohmann::detail
#endif

@nlohmann
Copy link
Owner Author

@YarikTH Thanks for the hints. The current state was a lot of fiddling, and I am currently happy with the way it is. That said, if you have any proposals how to improve, feel free to open a PR.

jpsim added a commit to jpsim/envoy-mobile that referenced this pull request Jan 28, 2022
The patch is no longer needed as it was similarly patched upstream in
nlohmann/json#3101.

That patch was then shipped in v3.10.5, which is the version used in Envoy now.

Signed-off-by: JP Simard <jp@jpsim.com>
jpsim added a commit to jpsim/envoy-mobile that referenced this pull request Jan 28, 2022
The patch is no longer needed as it was similarly patched upstream in
nlohmann/json#3101.

That patch was then shipped in v3.10.5, which is the version used in Envoy now.

Signed-off-by: JP Simard <jp@jpsim.com>
junr03 pushed a commit to envoyproxy/envoy-mobile that referenced this pull request Feb 1, 2022
The patch is no longer needed as it was similarly patched upstream in
nlohmann/json#3101.

That patch was then shipped in v3.10.5, which is the version used in Envoy now.

Signed-off-by: JP Simard <jp@jpsim.com>
jpsim added a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
The patch is no longer needed as it was similarly patched upstream in
nlohmann/json#3101.

That patch was then shipped in v3.10.5, which is the version used in Envoy now.

Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment