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

Error on assigment to std::string lvalue reference #450

Closed
VaderDev opened this issue Jul 9, 2017 · 11 comments
Closed

Error on assigment to std::string lvalue reference #450

VaderDev opened this issue Jul 9, 2017 · 11 comments
Assignees
Milestone

Comments

@VaderDev
Copy link

VaderDev commented Jul 9, 2017

sol::state lua;
lua.script("var = \"hi\"");

std::string var0;
var0 = lua["var"]; // error: call of overloaded 'assign(sol::proxy<sol::basic_table_core<true, sol::reference>&, const char (&)[4]>&)' is ambiguous
std::string var1 = lua["var"]; // good
@VaderDev
Copy link
Author

VaderDev commented Jul 9, 2017

Ohh, Lovely... It is using gcc 7.1.0 with C++17 enabled.

The problem seams to be that the standard implementation has some issues with string_view and basic_string overloads. I wonder if a workaround is even possible in usercode (without to much pain).

@VaderDev
Copy link
Author

VaderDev commented Jul 9, 2017

Duplicate of #414

@VaderDev VaderDev closed this as completed Jul 9, 2017
@VaderDev
Copy link
Author

VaderDev commented Jul 9, 2017

It turned out the usercode workaround is viable:
An additional sfiane for disabling std::string_view conversion in proxy_base if C++17 is present.
I don't have the time for a proper pull request nor proper testing, so be careful.

For reading from lua I dont think the general use case would involve std::string_view due to lifetime issues.

Big thanks for the folks at nlohmann/json/issues/464 for the macro

template<typename T,
	meta::enable<meta::neg<meta::is_string_constructible<T>>, meta::neg<is_proxy_primitive<meta::unqualified_t<T>>>> = meta::enabler
#if (defined(__cplusplus) && __cplusplus == 201703L) || (defined(_MSC_VER) && _MSC_VER >1900 && defined(_HAS_CXX17) && _HAS_CXX17 == 1)
	, typename = std::enable_if_t<!std::is_same_v<T, typename std::string_view>>
#endif
>
operator T& () const {
	const Super& super = *static_cast<const Super*>(static_cast<const void*>(this));
	return super.template get<T&>();
}

@VaderDev VaderDev reopened this Jul 9, 2017
@ThePhD
Copy link
Owner

ThePhD commented Jul 9, 2017

I forgot about this.

But yes, I need to add that is_string_view thing to the meta::is_string_constructible trait when I detect someone has C++17 or greater. I was hesitant to do so because VC++ still doesn't have header-inclusion-test-macros nor feature-test macros, so it's janky to do the test (as you can see from Modern JSON's big ol' macro work around).

I'll integrate this into the code-base and it should make things clean everywhere. Sorry about the problems with C++17-and-beyond stuff: it's still a WIP!

@VaderDev
Copy link
Author

VaderDev commented Jul 9, 2017

I don't think you have to apologize. It was a silent backward compatibility breaking change and I am still not convinced that the string_view works as intended in this corner case.

Thank you for your hard work.

@ThePhD
Copy link
Owner

ThePhD commented Jul 9, 2017

So here's a juicy side note: nlohmann's code is correct, insofar as you actually include a std library header. If you don't, _HAS_CXX_17 doesn't exist, and code that attempts to inspect it without including an MSVC header will barf.

@ThePhD
Copy link
Owner

ThePhD commented Jul 9, 2017

Check the latest, it should be a little bit more okay now.

@ThePhD ThePhD closed this as completed Jul 9, 2017
@ThePhD ThePhD self-assigned this Jul 9, 2017
@ThePhD ThePhD added this to the C++21 milestone Jul 9, 2017
@VaderDev
Copy link
Author

VaderDev commented Jul 9, 2017

With 48fc90b

#include <iostream>
#include <sol/state.hpp>

int main(int, char**) {
	sol::state lua;

	lua.script("var = \"world\"");

	std::string x;
	x = lua["var"];

	std::cout << x << std::endl;
}

Results in:

In file included from project/ext/sol/include/sol/usertype.hpp:26:0,
                 from project/ext/sol/include/sol/table_core.hpp:28,
                 from project/ext/sol/include/sol/table.hpp:25,
                 from project/ext/sol/include/sol/state_view.hpp:26,
                 from project/ext/sol/include/sol/state.hpp:25,
                 from project/sandbox/lua.cpp:4:
project/ext/sol/include/sol/usertype_metatable.hpp: In function 'std::__cxx11::string sol::usertype_detail::make_string(Arg&&)':
project/ext/sol/include/sol/usertype_metatable.hpp:216:25: error: 'sol::string_detail::string_shim {aka class std::basic_string_view<char>}' has no member named 'c_str'; did you mean '_M_str'?
    return std::string(s.c_str(), s.size());
                         ^~~~~
                         _M_str
In file included from project/ext/sol/include/sol/usertype.hpp:27:0,
                 from project/ext/sol/include/sol/table_core.hpp:28,
                 from project/ext/sol/include/sol/table.hpp:25,
                 from project/ext/sol/include/sol/state_view.hpp:26,
                 from project/ext/sol/include/sol/state.hpp:25,
                 from project/sandbox/lua.cpp:4:
project/ext/sol/include/sol/simple_usertype_metatable.hpp: In member function 'void sol::simple_usertype_metatable<T>::add(lua_State*, N&&, sol::constructor_wrapper<Fxs ...>)':
project/ext/sol/include/sol/simple_usertype_metatable.hpp:266:71: error: expected primary-expression before '>' token
    object o(L, in_place<detail::tagged<T, constructor_wrapper<Fxs...>>>, std::move(c));
                                                                       ^
project/ext/sol/include/sol/simple_usertype_metatable.hpp:266:72: error: expected primary-expression before ',' token
    object o(L, in_place<detail::tagged<T, constructor_wrapper<Fxs...>>>, std::move(c));
                                                                        ^
project/ext/sol/include/sol/simple_usertype_metatable.hpp: In member function 'void sol::simple_usertype_metatable<T>::add(lua_State*, N&&, sol::constructor_list<Lists ...>)':
project/ext/sol/include/sol/simple_usertype_metatable.hpp:276:70: error: expected primary-expression before '>' token
    object o(L, in_place<detail::tagged<T, constructor_list<Lists...>>>, std::move(c));
                                                                      ^
project/ext/sol/include/sol/simple_usertype_metatable.hpp:276:71: error: expected primary-expression before ',' token
    object o(L, in_place<detail::tagged<T, constructor_list<Lists...>>>, std::move(c));
                                                                       ^
project/ext/sol/include/sol/simple_usertype_metatable.hpp: In member function 'void sol::simple_usertype_metatable<T>::add(lua_State*, N&&, sol::destructor_wrapper<void>)':
project/ext/sol/include/sol/simple_usertype_metatable.hpp:286:68: error: expected primary-expression before '>' token
    object o(L, in_place<detail::tagged<T, destructor_wrapper<void>>>, std::move(c));
                                                                    ^
project/ext/sol/include/sol/simple_usertype_metatable.hpp:286:69: error: expected primary-expression before ',' token
    object o(L, in_place<detail::tagged<T, destructor_wrapper<void>>>, std::move(c));
                                                                     ^
project/ext/sol/include/sol/simple_usertype_metatable.hpp: In member function 'void sol::simple_usertype_metatable<T>::add(lua_State*, N&&, sol::destructor_wrapper<Fx>)':
project/ext/sol/include/sol/simple_usertype_metatable.hpp:296:66: error: expected primary-expression before '>' token
    object o(L, in_place<detail::tagged<T, destructor_wrapper<Fx>>>, std::move(c));
                                                                  ^
project/ext/sol/include/sol/simple_usertype_metatable.hpp:296:67: error: expected primary-expression before ',' token
    object o(L, in_place<detail::tagged<T, destructor_wrapper<Fx>>>, std::move(c));
                                                                   ^

@VaderDev
Copy link
Author

VaderDev commented Jul 9, 2017

The problem is unrelated of any code, the sole inclusion produces the same error.

@ThePhD
Copy link
Owner

ThePhD commented Jul 9, 2017

Sorry about that: I'm still vetting C++17 support. It's fixed now.

@VaderDev
Copy link
Author

Fix confirmed. Thank you.

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

2 participants