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

Cannot index by key of type static constexpr const char* #171

Closed
poskadesign opened this issue Jan 4, 2016 · 12 comments
Closed

Cannot index by key of type static constexpr const char* #171

poskadesign opened this issue Jan 4, 2016 · 12 comments

Comments

@poskadesign
Copy link

poskadesign commented Jan 4, 2016

This will not compile, error: use of overloaded operator '[]' is ambiguous (with operand types 'json' (aka 'basic_json<>') and 'const char *const'). Code:

static constexpr const char\* _VAR1 = "MyKey";
void foo() {
    nlohmann::json j;
    j[_VAR1] = 10;
}
@nlohmann
Copy link
Owner

nlohmann commented Jan 4, 2016

I can confirm the bug for Clang (Apple LLVM version 7.0.2 (clang-700.1.81)) and Clang 3.7:

Code:

#include <json.hpp>

using nlohmann::json;

int main()
{
    static constexpr const char* _VAR1 = "MyKey";

    nlohmann::json j;
    j[_VAR1] = 10;
}

yields:

issue171.cpp:10:6: error: use of overloaded operator '[]' is ambiguous (with operand types 'nlohmann::json' (aka 'basic_json<>') and 'const char *const')
    j[_VAR1] = 10;
    ~^~~~~~
src/json.hpp:2911:15: note: candidate function
    reference operator[](const typename object_t::key_type& key)
              ^
src/json.hpp:2959:21: note: candidate function
    const_reference operator[](const typename object_t::key_type& key) const
                    ^
issue171.cpp:10:6: note: built-in candidate operator[](long, const char *)
    j[_VAR1] = 10;
     ^
issue171.cpp:10:6: note: built-in candidate operator[](long, const volatile char *)
1 error generated.
make: *** [issue171] Error

Interestingly, GCC 5.2.0 works... I shall have a look at this later.

@nlohmann
Copy link
Owner

nlohmann commented Jan 4, 2016

@nlohmann
Copy link
Owner

nlohmann commented Jan 4, 2016

The ambiguity is not really between the const and the non-const version. A different program yields

src/json.hpp:2959:21: note: candidate function
    const_reference operator[](const typename object_t::key_type& key) const
                    ^
issue171_1.cpp:13:15: note: built-in candidate operator[](long, const char *)
    auto x = k[_VAR1];
              ^
issue171_1.cpp:13:15: note: built-in candidate operator[](long, const volatile char *)

@gregmarr
Copy link
Contributor

gregmarr commented Jan 4, 2016

Is the entire root of the ambiguity this function:

operator value_t() const noexcept
{
    return m_type;
}

which allows a json to be converted to an enum? If so, should that just be removed in favor of the explicit type() function?

@nlohmann
Copy link
Owner

nlohmann commented Jan 4, 2016

I'll look into it tomorrow.

@nlohmann
Copy link
Owner

I removed operator value_t() const noexcept and get the same error.

Then I tried removing operator ValueType() const and the code compiles. It seems to be the same issue as in http://stackoverflow.com/questions/8914986/should-this-compile-overload-resolution-and-implicit-conversions

@nlohmann
Copy link
Owner

After reading http://clang-developers.42468.n3.nabble.com/Why-doesn-t-this-compile-clang-implicit-conversions-amp-overload-resolution-td3668157.html, I added the constraint and not std::is_same<ValueType, long>::value to operator ValueType() const and the example compiles...

@nlohmann
Copy link
Owner

It seems like Clang is correct to reject the code, and the only fix I could think of is to forbid an implicit conversion of a json object to long. However, I think the latter usecase is way more common than the issue we try to fix...

In the end, j[_VAR1] = 10; could be replaced by j[std::string(_VAR1)] = 10;.

What do you think?

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Jan 10, 2016
@nlohmann
Copy link
Owner

Right now, I am clueless how to fix the issue. As The use case is rather specific, I could live with closing the issue as "won't fix".

@twelsby
Copy link
Contributor

twelsby commented Jan 22, 2016

I have a solution for this. It turns out that constexpr and static are irrelevant to the problem, which can be reduced to:

const char* _VAR1 = "MyKey";
nlohmann::json j;
j[_VAR1] = 10;

In addition the following also cause the same problem (c style casts for brevity):

char* _VAR1 = "MyKey";
j[_VAR1] = 10;
j[(const char*)"MyKey"] = 10;
j[(char*)"MyKey"] = 10;

Yet these all work:

char _VAR1[] = "MyKey";
j[_VAR1] = 10;
const char _VAR2[] = "MyKey";
j[_VAR2] = 10;
j[(const char)"MyKey"] = 10;
j[(char)"MyKey"] = 10;

There is a clear pattern. The signature of the template we want to instantiate (the path it takes when it works) is this or its const_reference counterpart:

template<typename T, std::size_t n>
reference operator[](const T (&key)[n])

It would seem that this template is being rejected because for a char* there won't be a second template parameter. The compiler then ignores this one and looks to all the single parameter templates, which are all equally bad (taking integers/enums) so it gives up.

The solution it would seem is to create another two template overrides in the general form:

template<typename T>
reference operator[](T* key)

To improve code reuse it would be better to put the actual code in this one and then call it from the char array one. There needs to be a complementary one for the const_reference case.

Add this in and it compiles under both gcc and VS2015. I've pushed this to my repo if you want to see it in action. I also added some extra unit tests to check all the combinations. AppVeyor and Travis continue to pass all tests.

@nlohmann nlohmann removed the state: please discuss please discuss the issue or vote for your favorite option label Jan 22, 2016
@nlohmann nlohmann added this to the Release 1.0.1 milestone Jan 22, 2016
@nlohmann
Copy link
Owner

Hi @twelsby, thanks for the further analysis and solution. I was missing the fact that char[] in fact worked...

@nlohmann
Copy link
Owner

@twelsby, would it be possible to open a separate PR for this issue?

twelsby pushed a commit to twelsby/json that referenced this issue Jan 23, 2016
nlohmann added a commit that referenced this issue Jan 24, 2016
Fixed Issue #171 - added two extra template overloads of operator[] for T* arguments
nlohmann added a commit that referenced this issue Jul 8, 2017
It seems these functions are not required any more. The code was added in commit 7e32457 to fix issue #171. There are still regression tests for #171, so when this commit passes the CI, the functions may be removed for good.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants