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

gcc13.2 compile fail when using std::string to compare the json object #4165

Open
zhangjian3032 opened this issue Sep 26, 2023 · 6 comments
Open
Labels
confirmed kind: bug state: help needed the issue needs help to proceed

Comments

@zhangjian3032
Copy link

Description

Kindly refer to this link: https://godbolt.org/z/c3fzjnqzG,
okay with gcc 12.3
failed with gcc 13.1 and 13.2

Reproduction steps

refer to description

Expected vs. actual results

expect using gcc13.2 can be compiled successfully

    if (std::string("bar") == j["xxx"])
    {
       
    }

Minimal code example

#include <iostream>
#include <string>
#include <nlohmann/json.hpp>

int main()
{
    nlohmann::json j;


    if (std::string("bar") == j["xxx"])
    {
       
    }
    
    return 0;
}


### Error messages

_No response_

### Compiler and operating system

gcc12.3 and gcc13.1

### Library version

3.11.1

### Validation

- [ ] The bug also occurs if the latest version from the [`develop`](https://github.com/nlohmann/json/tree/develop) branch is used.
- [ ] I can successfully [compile and run the unit tests](https://github.com/nlohmann/json#execute-unit-tests).
@gregmarr
Copy link
Contributor

This only happens with C++20 mode enabled, and appears to be a result of the updated C++ compare code. It works in one direction but not the other.

This fails

if (std::string("bar") == j["xxx"])

This is good

if (j["xxx"] == std::string("bar"))

@colbychaskell
Copy link
Contributor

colbychaskell commented Oct 5, 2023

I can confirm I get the same behavior running on ubuntu 23.04 with gcc 13.2.

It looks like the reason why this only appears for c++20 is the template requirement for the operator==

template<typename ScalarType>
requires std::is_scalar_v<ScalarType>
bool operator==(ScalarType rhs) const noexcept
{
    return *this == basic_json(rhs);
}

If the 'requires' line is commented out it appears to use this version of the == operator for this case.

I believe the reason why it works when lhs and rhs are swapped is because of the operator value_t() definition which lets it work with the std::string == operator.

I'm not sure why this function isn't just setup to work with all compatible types which would work with the basic_json(rhs) conversion?

@nlohmann
Copy link
Owner

nlohmann commented Oct 5, 2023

I did not write this code myself, and did not find the time to read into these operators. Help is more than welcome here!

@nlohmann nlohmann added confirmed state: help needed the issue needs help to proceed labels Oct 5, 2023
@fsandhei
Copy link

fsandhei commented Oct 7, 2023

Without having worked on the library details I cannot give good rationales, but I decided to try out changing the constraint so operator== is called for the compatible types, like the equivalent catch - all constructor.

@@ -3694,9 +3694,11 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec

     /// @brief comparison: equal
     /// @sa https://json.nlohmann.me/api/basic_json/operator_eq/
-    template<typename ScalarType>
-    requires std::is_scalar_v<ScalarType>
-    bool operator==(ScalarType rhs) const noexcept
+    template < typename CompatibleType,
+               typename U = detail::uncvref_t<CompatibleType>,
+               detail::enable_if_t <
+                   !detail::is_basic_json<U>::value && detail::is_compatible_type<basic_json_t, U>::value, int > = 0 >
+    bool operator==(CompatibleType rhs) const noexcept
     {
         return *this == basic_json(rhs);
     }

The minimal example compiles (with GCC 13.2.1 on Arch) and the tests pass on develop.

There might be some details that I'm missing here, like unforeseen effects of this, so any input is appreciated. If this is okay I'll gladly make a PR for it.

@schummLee
Copy link

thats becauseIn GCC 13.1 and 13.2, stricter checks might be in place that catch potential issues earlier. It could be related to the handling of non-existing keys in a nlohmann::json object. Accessing a non-existing key with the operator[] in nlohmann::json returns a nullptr, which may not have a direct comparison overload with std::string in later versions of the library or compiler, I ALSO HAV THE CODE REFER TO THIS

if u using .find(),this error wont occur in here
auto it = j.find("xxx");
if (it != j.end() && *it == std::string("bar")) {

}
if (j["xxx"].is_string() && std::string("bar") == j["xxx"].getstd::string()) {

}

@schummLee
Copy link

I can confirm I get the same behavior running on ubuntu 23.04 with gcc 13.2.

It looks like the reason why this only appears for c++20 is the template requirement for the operator==

template<typename ScalarType>
requires std::is_scalar_v<ScalarType>
bool operator==(ScalarType rhs) const noexcept
{
    return *this == basic_json(rhs);
}

If the 'requires' line is commented out it appears to use this version of the == operator for this case.

I believe the reason why it works when lhs and rhs are swapped is because of the operator value_t() definition which lets it work with the std::string == operator.

I'm not sure why this function isn't just setup to work with all compatible types which would work with the basic_json(rhs) conversion?

When you write j["xxx"] == std::string("bar"), the operator== from the nlohmann::json library gets invoked, but due to the constrained template, it doesn't find a suitable match for a non-scalar right-hand side. This is different from std::string("bar") == j["xxx"], where the operator== for std::string comes into play, and the nlohmann::json object gets implicitly converted to std::string (if it can be), making the comparison possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed kind: bug state: help needed the issue needs help to proceed
Projects
None yet
Development

No branches or pull requests

6 participants