-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
VS2017 implicit to std::string conversion fix. #464
Comments
My bad - I misunderstood your comment, so I reverted the last commit. I cannot add the line with Could you please post the compiler error for the original code together with the options you are using with VS2017? |
Here are my tests with 1a6d7f5: #include <json.hpp>
#include <string>
int main(int argc, char* argv[]) {
using json = nlohmann::json;
json v = "test";
std::string test;
test = v;
return 0;
} Compiled with
I propose this change: template < typename ValueType, typename std::enable_if <
not std::is_pointer<ValueType>::value and
not std::is_same<ValueType, typename string_t::value_type>::value
#if !defined(_MSC_VER) || _MSC_VER > 1900 // fix for issue #167 operator<< ambiguity under VS2015
and not std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>::value
#endif
#if defined(_MSC_VER) && defined(_HAS_CXX17)
and not std::is_same<ValueType, typename std::string_view>::value
#endif
, int >::type = 0 >
operator ValueType() const
{
// delegate the call to get<>() const
return get<ValueType>();
} The |
I shall install VS2017 and investigate that bug, we cannot put Thanks for the report! |
Thanks for the feedback. This seems to be an MSVC bug, since the code compiles with earlier versions and C++17 should not break C++11 code. |
I'm not sure there is a good way to fix this problem, #144 fixed a similar issue, by removing the possibility to convert If I understood correctly the other issues, this problem will arise whenever a class with several To me, #144 was not a good fix, for the reason mentioned above, thus I don't want to do the same thing.
That might not suit your needs, but I really don't know if there's a way to workaround that without removing features like in #144 We should mention this problem in the README, since this might come up again, even with user-defined types. |
FYI: The library is now also checked with MSVC 19.10.25017.0 / Build Engine version 15.1.548.43366 at AppVeyor, see https://ci.appveyor.com/project/nlohmann/json. |
@qis I added the code you posted in #464 (comment) as regression test (b4dbebf). It succeeds with MSVC 2015 and 2017 in AppVeyor. Can I close this ticket? |
@nlohmann Great! May I have until evening (UTC) to test it? Just in case I find a related problem. |
Of course! |
@nlohmann Ok, it works fine (like before) if there is no Here is a complete project for the minimal example: https://github.com/qis/json P.S.: Just noticed that |
Sorry, I misunderstood. I shall add your proposed fix. I also need to add a c++latest build to AppVeyor. |
@qis I am neither expert with MSVC not with CMake. Could you give me a hint how to pass |
@nlohmann You can just add it to |
To test a fix for issue #464 (not yet implemented), we first need to have an MSVC build with “/permissive- /std:c++latest /utf-8”.
Ok, got it. AppVeyor now has a configuration where MSVC 2017 is executed with the mentioned flags: https://ci.appveyor.com/project/nlohmann/json/build/1882/job/xmj9j1tlga6eeh46 Next stop: adding the fix ;) |
Unfortunately, the proposed fix does not work, see 628be15 and https://ci.appveyor.com/project/nlohmann/json/build/1884. It seems as if
I have to find a better way to check whether |
@nlohmann Seems to work fine in both, VS2017 and clang 4.0.0 in C++17 mode. Thank you very much for this change! |
Merged the feature branch. Thanks for reporting and helping out so patiently! |
doesn't work in VS2015.3 ,with /std:c++latest |
Could you provide an error message and the exact version? |
json.hpp(3734): error C2039: 'string_view': is not a member of 'std'
|
@lethe555 The |
@qis YES . only check the value of _HAS_CXX17 is not enough |
I'll have a look. |
needed for VS2015.3 with /std:c++latest
@lethe555 I added your proposed fix. Could you please check if it works for you? |
@nlohmann It works well now. |
Cool, thanks! |
Copied from solution to nlohmann#464
Hi! Great work so far. I had to patch the library to allow implicit to-string-conversions in VS2017.
Hope this helps.
The text was updated successfully, but these errors were encountered: