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

build failure on upcoming gcc-12: test/src/unit-regression1.cpp:392:22: error: ambiguous overload for 'operator=' #3138

Closed
1 task done
trofi opened this issue Nov 13, 2021 · 27 comments · Fixed by #3379
Closed
1 task done
Assignees
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@trofi
Copy link
Contributor

trofi commented Nov 13, 2021

On gcc-12 (20211107 snapshot) std::string amended it's constructors to a form that fails regression tests building:

mkdir b && cd b
cmake ..
make
...
[ 71%] Building CXX object test/CMakeFiles/test-regression1.dir/src/unit-regression1.cpp.o
/home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp: In function 'void _DOCTEST_ANON_FUNC_2()':
/home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:392:22: error: ambiguous overload for 'operator=' (operand types are 'std::string' {aka 'std::__cxx11::basic_string<char>'} and 'nlohmann::basic_json<>::value_type' {aka 'nlohmann::basic_json<>'})
  392 |         s2 = o["name"];
      |                      ^
In file included from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/string:53,
                 from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/locale_classes.h:40,
                 from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/locale:39,
                 from /home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:33:
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:627:21: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]' (deleted)
  627 |       basic_string& operator=(nullptr_t) = delete;
      |                     ^~~~~~~~
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:682:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  682 |       operator=(const basic_string& __str)
      |       ^~~~~~~~
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:720:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  720 |       operator=(basic_string&& __str)
      |       ^~~~~~~~
In file included from /home/slyfox/dev/git/nlohmann-json/test/thirdparty/doctest/doctest_compatibility.h:6,
                 from /home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:30:
/home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:398:37: error: ambiguous overload for 'operator=' (operand types are 'std::string' {aka 'std::__cxx11::basic_string<char>'} and 'nlohmann::basic_json<>::value_type' {aka 'nlohmann::basic_json<>'})
  398 |         CHECK_THROWS_AS(s2 = o["int"], json::type_error);
      |                                     ^
/home/slyfox/dev/git/nlohmann-json/test/thirdparty/doctest/doctest.h:1844:23: note: in definition of macro 'DOCTEST_CAST_TO_VOID'
 1844 |     static_cast<void>(__VA_ARGS__);                                                                \
      |                       ^~~~~~~~~~~
/home/slyfox/dev/git/nlohmann-json/test/thirdparty/doctest/doctest.h:2163:44: note: in expansion of macro 'DOCTEST_ASSERT_THROWS_AS'
 2163 | #define DOCTEST_CHECK_THROWS_AS(expr, ...) DOCTEST_ASSERT_THROWS_AS(expr, DT_CHECK_THROWS_AS, "", __VA_ARGS__)
      |                                            ^~~~~~~~~~~~~~~~~~~~~~~~
/home/slyfox/dev/git/nlohmann-json/test/thirdparty/doctest/doctest.h:2576:36: note: in expansion of macro 'DOCTEST_CHECK_THROWS_AS'
 2576 | #define CHECK_THROWS_AS(expr, ...) DOCTEST_CHECK_THROWS_AS(expr, __VA_ARGS__)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~
/home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:398:9: note: in expansion of macro 'CHECK_THROWS_AS'
  398 |         CHECK_THROWS_AS(s2 = o["int"], json::type_error);
      |         ^~~~~~~~~~~~~~~
In file included from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/string:53,
                 from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/locale_classes.h:40,
                 from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/locale:39,
                 from /home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:33:
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:627:21: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]' (deleted)
  627 |       basic_string& operator=(nullptr_t) = delete;
      |                     ^~~~~~~~
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:682:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  682 |       operator=(const basic_string& __str)
      |       ^~~~~~~~
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:720:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  720 |       operator=(basic_string&& __str)
      |       ^~~~~~~~
In file included from /home/slyfox/dev/git/nlohmann-json/test/thirdparty/doctest/doctest_compatibility.h:6,
                 from /home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:30:
/home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:402:39: error: ambiguous overload for 'operator=' (operand types are 'std::string' {aka 'std::__cxx11::basic_string<char>'} and 'nlohmann::basic_json<>::value_type' {aka 'nlohmann::basic_json<>'})
  402 |         CHECK_THROWS_WITH(s2 = o["int"], "[json.exception.type_error.302] type must be string, but is number");
      |                                       ^
/home/slyfox/dev/git/nlohmann-json/test/thirdparty/doctest/doctest.h:1844:23: note: in definition of macro 'DOCTEST_CAST_TO_VOID'
 1844 |     static_cast<void>(__VA_ARGS__);                                                                \
      |                       ^~~~~~~~~~~
/home/slyfox/dev/git/nlohmann-json/test/thirdparty/doctest/doctest.h:2167:46: note: in expansion of macro 'DOCTEST_ASSERT_THROWS_WITH'
 2167 | #define DOCTEST_CHECK_THROWS_WITH(expr, ...) DOCTEST_ASSERT_THROWS_WITH(expr, #expr, DT_CHECK_THROWS_WITH, __VA_ARGS__)
      |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/slyfox/dev/git/nlohmann-json/test/thirdparty/doctest/doctest.h:2577:38: note: in expansion of macro 'DOCTEST_CHECK_THROWS_WITH'
 2577 | #define CHECK_THROWS_WITH(expr, ...) DOCTEST_CHECK_THROWS_WITH(expr, __VA_ARGS__)
      |                                      ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:402:9: note: in expansion of macro 'CHECK_THROWS_WITH'
  402 |         CHECK_THROWS_WITH(s2 = o["int"], "[json.exception.type_error.302] type must be string, but is number");
      |         ^~~~~~~~~~~~~~~~~
In file included from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/string:53,
                 from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/locale_classes.h:40,
                 from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/locale:39,
                 from /home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:33:
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:627:21: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]' (deleted)
  627 |       basic_string& operator=(nullptr_t) = delete;
      |                     ^~~~~~~~
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:682:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  682 |       operator=(const basic_string& __str)
      |       ^~~~~~~~
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:720:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  720 |       operator=(basic_string&& __str)
      |       ^~~~~~~~
/home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:1174:16: error: ambiguous overload for 'operator=' (operand types are 'std::string' {aka 'std::__cxx11::basic_string<char>'} and 'nlohmann::json' {aka 'nlohmann::basic_json<>'})
 1174 |         test = v;
      |                ^
In file included from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/string:53,
                 from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/locale_classes.h:40,
                 from /nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/locale:39,
                 from /home/slyfox/dev/git/nlohmann-json/test/src/unit-regression1.cpp:33:
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:627:21: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]' (deleted)
  627 |       basic_string& operator=(nullptr_t) = delete;
      |                     ^~~~~~~~
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:682:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  682 |       operator=(const basic_string& __str)
      |       ^~~~~~~~
/nix/store/0hvbmcf38gqpr6c49h7zl3iy228afaz0-gcc-12.0.0/include/c++/12.0.0/bits/basic_string.h:720:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  720 |       operator=(basic_string&& __str)
      |       ^~~~~~~~
make[2]: *** [test/CMakeFiles/test-regression1.dir/build.make:76: test/CMakeFiles/test-regression1.dir/src/unit-regression1.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:2148: test/CMakeFiles/test-regression1.dir/all] Error 2
make: *** [Makefile:146: all] Error 2

Which compiler and operating system are you using?

  • Compiler: gcc-12 development snapshot
  • Operating system: x86_64-linux

Which version of the library did you use?

  • the develop branch
@trofi trofi changed the title build failure on upcoming gcc-12: test/src/unit-regression1.cpp:392:22: error: ambiguous overload for 'operator=' (operand types are 'std::string' {aka 'std::__cxx11::basic_string<char>'} and 'nlohmann::basic_json<>::value_type' {aka 'nlohmann::basic_json<>'}) build failure on upcoming gcc-12: test/src/unit-regression1.cpp:392:22: error: ambiguous overload for 'operator=' Nov 13, 2021
@nlohmann
Copy link
Owner

I would like to test this in the CI - can you tell me how I can get a development snapshot of GCC, preferably for Ubuntu?

@trofi
Copy link
Contributor Author

trofi commented Nov 20, 2021

Oh, that's a good question. Looking around ubuntu has https://launchpad.net/ubuntu/+source/gcc-snapshot. Looking at impish release latest release there is 20210827. It does not yet have a std::string change added on Oct 1 in https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=cf876562c592193732f869e9f96034a42d0fad89 . I hope it eventually catches up.

From superficial test in VM it's enough to install it as:

$ apt install gcc-snapshot
$ export PATH=/usr/lib/gcc-snapshot/bin:$PATH
$ cmake ...

I personally build gcc locally from https://ftp.mpi-inf.mpg.de/mirrors/gnu/mirror/gcc.gnu.org/pub/gcc/snapshots/LATEST-12/ (Current is from Oct 14).

@nlohmann
Copy link
Owner

Thanks, I was not aware of package gcc-snapshot.

@nlohmann
Copy link
Owner

I can reproduce the issue with gcc-latest (GCC) 12.0.0 20211114 (experimental).

@nlohmann
Copy link
Owner

The failing regression tests are for #144 and #464.

@nlohmann nlohmann added the state: help needed the issue needs help to proceed label Nov 21, 2021
@nlohmann
Copy link
Owner

Same error occurred again in #3226

@musicinmybrain
Copy link
Contributor

GCC 12 is now in Fedora Rawhide, if that helps with testing.

(I found this bug because it’s causing https://github.com/monocasual/giada builds to fail in Fedora Rawhide.)

@xvitaly
Copy link
Contributor

xvitaly commented Jan 16, 2022

I can confirm. All dependent packages are now FTBFS.

@nlohmann
Copy link
Owner

That's too bad. I have not yet understood the reason for the error, let alone whether it's a bug in the library and how to fix it.

@jwakely
Copy link

jwakely commented Jan 17, 2022

https://jwakely.github.io/pkg-gcc-latest/ has GCC snapshots packages for Ubuntu, Fedora, RHEL/CentOS and Tumbleweed.

The error is due to GCC 12 implementing P2166R1 "Prohibit std::basic_string and std::basic_string_view construction from nullptr", which is required by the C++23 draft. I made the change apply unconditionally to older standards too, because assigning nullptr to a basic_string was always undefined behaviour anyway. Apparently it changes the overload set for your library, causing an error during overload resolution.

@nlohmann
Copy link
Owner

As described in https://en.cppreference.com/w/cpp/23, we will have the same problem with Clang libc++ 13 and MSVC STL 19.30. What I have not understood is whether deleting basic_string(nullptr_t) is actually meant to also affect C++11/C++14/C++17 builds.

@t-b
Copy link
Contributor

t-b commented Jan 18, 2022

What I have not understood is whether deleting basic_string(nullptr_t) is actually meant to also affect C++11/C++14/C++17 builds.

I have found https://www.kitware.com/itk-informs-c-stl-development/ a while ago and from that I would say this is only deleted in C++23 builds. See also microsoft/STL#1995 (comment) from linked from there.

@jwakely
Copy link

jwakely commented Jan 18, 2022

The deleted basic_string(nullptr) constructor is new in C++23, but constructing a std::string from nullptr was always undefined behaviour, so I made it present unconditionally for C++11/14/17/20 in GCC's std::lib. I can't understand what the conversions on your code are supposed to be doing, so I can't tell whether your code was always doing something bad and it's only being diagnosed now, or if the new constructor changes the behaviour of correct code.

Could you give a simplified example of what conversion is supposed to happen when constructing a std::string from a basic_json<>?

@jwakely
Copy link

jwakely commented Jan 18, 2022

I think this is probably an approximation of what you're doing, right?

#include <string>

struct S
{
  operator const char*() const { return ""; }
  operator std::nullptr_t() const { return {}; }
};

std::string s{ S{} };

This is valid in C++20 and earlier, but ill-formed in C++23. And it demonstrates that the change should be limited to C++23. I'll change GCC.

@jwakely
Copy link

jwakely commented Jan 18, 2022

Filed as a GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104099

@t-b
Copy link
Contributor

t-b commented Jan 18, 2022

I've compiled latest gcc-trunk locally here g++ (GCC) 12.0.1 20220118 (experimental) and with the following patch

diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp
index 92308c557..8fd2b357f 100644
--- a/single_include/nlohmann/json.hpp
+++ b/single_include/nlohmann/json.hpp
@@ -19043,6 +19043,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     template < typename ValueType, typename std::enable_if <
                    detail::conjunction <
                        detail::negation<std::is_pointer<ValueType>>,
+                       detail::negation<std::is_same<ValueType, nullptr_t>>,
                        detail::negation<std::is_same<ValueType, detail::json_ref<basic_json>>>,
                                         detail::negation<std::is_same<ValueType, typename string_t::value_type>>,
                                         detail::negation<detail::is_basic_json<ValueType>>,

it compiles and all tests pass. The fix goes in the same direction as #3138 (comment). It would have helped me if I read the reply first before trying to fix it ;)

EDIT:
As explanation the above diff is from

json/include/nlohmann/json.hpp

Lines 1870 to 1890 in 293f67f

@since version 1.0.0
*/
template < typename ValueType, typename std::enable_if <
detail::conjunction <
detail::negation<std::is_pointer<ValueType>>,
detail::negation<std::is_same<ValueType, detail::json_ref<basic_json>>>,
detail::negation<std::is_same<ValueType, typename string_t::value_type>>,
detail::negation<detail::is_basic_json<ValueType>>,
detail::negation<std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>>,
#if defined(JSON_HAS_CPP_17) && (defined(__GNUC__) || (defined(_MSC_VER) && _MSC_VER >= 1910 && _MSC_VER <= 1914))
detail::negation<std::is_same<ValueType, std::string_view>>,
#endif
detail::is_detected_lazy<detail::get_template_function, const basic_json_t&, ValueType>
>::value, int >::type = 0 >
JSON_EXPLICIT operator ValueType() const
{
// delegate the call to get<>() const
return get<ValueType>();
}
and handles implicit conversion. I don't see a reason why we would like to convert to nullptr_t here.

nlohmann added a commit that referenced this issue Jan 19, 2022
@nlohmann
Copy link
Owner

Thanks @t-b for the fix! I tried it, and it worked for GCC 12. Unfortunately, it does not work for any other compiler :-/

See #3226.

The error is

/__w/json/json/single_include/nlohmann/json.hpp:19046:65: error: template argument for template type parameter must be a type
                       detail::negation<std::is_same<ValueType, nullptr_t>>,
                                                                ^~~~~~~~~

Any idea?

@gregmarr
Copy link
Contributor

Should be std::nullptr_t

nlohmann added a commit that referenced this issue Jan 19, 2022
@nlohmann
Copy link
Owner

Good catch! Totally overlooked that. :-/

@t-b
Copy link
Contributor

t-b commented Jan 19, 2022

Jeep std:: is missing, hacked it a bit too fast ;)

@nlohmann
Copy link
Owner

The code compiles, but there is one test failing when compiling the code with C++20:

TEST CASE:  lexicographical comparison operators
  values
  comparison: less

/__w/json/json/test/src/unit-comparison.cpp:213: ERROR: CHECK( (j_values[i] < j_values[j]) == expected[i][j] ) is NOT correct!
  values: CHECK( false == 1 )
  logged: i := 12
          j := 13
          j_values[i] := [1,2,3]
          j_values[j] := ["one","two","three"]

/__w/json/json/test/src/unit-comparison.cpp:213: ERROR: CHECK( (j_values[i] < j_values[j]) == expected[i][j] ) is NOT correct!
  values: CHECK( true == 0 )
  logged: i := 13
          j := 12
          j_values[i] := ["one","two","three"]
          j_values[j] := [1,2,3]

The same test succeeds with Clang (Linux, macOS) in C++20 mode. Any ideas?

@gregmarr
Copy link
Contributor

Looks like a change in how the default comparisons are happening in C++20 with the new generated comparisons. Looking at this in Compiler Explorer to see if I can track it down.

@gregmarr
Copy link
Contributor

gregmarr commented Jan 28, 2022

Confirmed different behavior of comparing std::vector<nlohmann::json<>> between -std=c++17 and -std=c++20 in GCC trunk.

The former calls std::operator<(vector<json>const&,vector<json>const&) and the latter calls std::operator<=>(vector<json>const&,vector<json>const&) so there is some difference in implementation there.

@nlohmann
Copy link
Owner

Would it make sense to implement operator<=> for basic_json instead of the usual operators when compiled for C++20?

@jwakely
Copy link

jwakely commented Jan 30, 2022

If you have implicit conversions then yes, you should provide your own <=>. Otherwise std::lib components that want to use <=> will do so via your implicit conversions, with possibly unexpected results.

@gregmarr
Copy link
Contributor

gregmarr commented Feb 3, 2022

I dug into that and still couldn't get it to pass. It would be good to replace all the comparison and equality operators with <=> for both the main class and the type enum, but it won't necessarily fix this issue.

falbrechtskirchinger pushed a commit to falbrechtskirchinger/json that referenced this issue Mar 4, 2022
@falbrechtskirchinger
Copy link
Contributor

I dug into that and still couldn't get it to pass. It would be good to replace all the comparison and equality operators with <=> for both the main class and the type enum, but it won't necessarily fix this issue.

Worse than "won't necessarily fix this issue", libcxx [1] doesn't fully implement three way comparison for various std types yet, creating more problems. I've tried.

So, we'll have to solve/work around the underlying issue using the existing operators first, then optionally add the spaceship operator and guard that with requires clauses to work around missing standard library support.

Guess I know how I'll spend the weekend. 😉

[1] https://libcxx.llvm.org/Status/Spaceship.html

falbrechtskirchinger pushed a commit to falbrechtskirchinger/json that referenced this issue Mar 4, 2022
falbrechtskirchinger pushed a commit to falbrechtskirchinger/json that referenced this issue Mar 5, 2022
falbrechtskirchinger pushed a commit to falbrechtskirchinger/json that referenced this issue Mar 6, 2022
falbrechtskirchinger pushed a commit to falbrechtskirchinger/json that referenced this issue Mar 6, 2022
falbrechtskirchinger pushed a commit to falbrechtskirchinger/json that referenced this issue Mar 6, 2022
@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed state: help needed the issue needs help to proceed labels Mar 7, 2022
@nlohmann nlohmann self-assigned this Mar 7, 2022
@nlohmann nlohmann added this to the Release 3.10.6 milestone Mar 7, 2022
nlohmann added a commit that referenced this issue Mar 7, 2022
* 🔧 use proper GCC binary

* 🔧 add more GCC warning flags

* ⚗️ try fix from #3138 (comment)

* Fix custom allocator test build failures (C++20)

Allocator tests fail to compile in C++20 mode with clang+MS STL due
to missing copy constructors.

* Fix test build failures due to missing noexcept (gcc-12)

* alt_string has multiple member functions that should be marked noexcept.
* nlohmann::ordered_map constructors can be noexcept.

Compilation failures result from the warning flag -Werror=noexcept and
gcc-12.

* Disable broken comparison tests in C++20 mode

Co-authored-by: Niels Lohmann <mail@nlohmann.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants