Using QString as string type #274
Comments
|
Could you please paste the compiler error you got when trying to use |
Since this is a private function that is only used with the std::domain_error() constructor, changing the return type of this function to std::string should be fine. Can you try that and see if there are any other errors? |
|
@gregmarr How comes that you are always that fast ;) |
|
@nlohmann I'm not sure this function can really be noexcept, since it creates a string object, which may need to allocate memory. |
|
I will try and see if that helps :-) |
|
I does help. |
|
Ok, some is working but this program using dump() gives a lot of compiler errors: using json = nlohmann::basic_json<std::map, std::vector, QString>;
using namespace std;
int main()
{
json j;
j["attrib1"] = "val1";
auto s = j.dump(4);
return 0;
} |
|
Errors here: I |
|
The first error is related to #268: It is implicitly assumed that Could you try to replace line 2099 with std::basic_stringstream<typename string_t::value_type,
typename string_t::traits_type,
typename string_t::allocator_type> ss;? |
|
I wll try :) |
|
Hi have been on holiday, but will get back to this problem and try you suggestion. |
|
Now it complains that QString does not have a traits_type nested type: |
|
It seems as if I thought about changing the |
|
Hi again Since we really would like to use this at work, I spent today hacking a bit in your code. Right now the policy class is itself a template, which is probably not strictly necessary. My only (small) "problem" is the string literal operators (operator "") you have provided which are very nice indeed. They are defined for a specific typedef of you template and thus conflicts if you use you own. For now I commented those operators out. A solution could be for you to provide those default typedefs in a separate header file which those wanting the default std::string based version, and the rest of us can just include the main header file and make those typedefs/using and operators ourselves. I suggest using a diff tool to compare my file with your original. Mine is called jsonqt.hpp, which is your original file with added support for instantiating with QString. To use your great template with Qt: Regards Martin Lütken |
|
Do you know what version of the library you used as your starting point? There's a lot of differences that don't look like they're things that you would have made. |
|
@mlutken I had a brief look at your code - I am not sure about the overhead of the abstraction, and I am also unsure whether QStrings justify adding another template parameter... Can you comment on that? |
|
Well. You already have a lot of template parameters. One of which is the string type. Seems that you can only use it with std:: string types... I really do prefer std:: stuff, but honestly std::string is really not adequate for many applications. QString is probably not perfect either, but it has all the things that std::string is missing like unicode (UTF-8 in particular). |
|
This I believe is the version I used as starting point. |
|
@nlohmann Is this a bug? s is copied into m_buffer, but then m_content, m_start, and m_cursor are set to point to the passed in string rather than the internal string. |
|
There is definitely a performance concern here: The first three introduce additional copies of the string which are not necessary StringType is std::string (#1) or when LexerStringType and StringType are the same (#2 and #3). Changing them to const & would help there. I'm not sure about #4. I think it's okay. FYI, you have a typo in a few places, there is only one t in literal. I can see why you needed additional support in dump to get from the type of std::stringstream::str, and that would probably be needed for string_t = std::wstring as well. What was the problem when the lexer's m_buffer was QString that it had to be changed to be std::string? |
|
Lexer seems to assume const char* is that not correct? |
|
#4 is the same your original code so that should execute at same speed. Right ? |
|
The bug you are asking about: The above is how the code I sent you should look like.... Is that not OK? |
|
Yes, that's why I noticed it, pointing it out to @nlohmann in case he didn't notice. Sorry that wasn't clear. |
Okay, so #1-#3 are currently only used in places that are already making a copy, so while it has the potential to be expensive in general, it probably isn't right now. However, adding const & on the output of those functions for the String version wouldn't cost anything. |
|
Do you mean like this: ? |
|
static const StringType& toString(const std::stringstream& ss) This will not work, since then we return a reference to a temporary, which is not good :) I am a little puzzled why you are so concerned about these function making copies. Also even if we were to make a copy of std::string somewhere, even that would be extremely fast in C++11 and forward since move semantics would apply here. It might be that in C#, Java etc. these functions would indeed be real functions and do indeed make real copies, but I am almost certain they will not do so in C++. Am I missing something here? |
|
Ah, I forgot that stringstream::str returns by value instead of by reference. I know that I would expect many things to be inlined and copies elided, but there have been many times where things don't always work as one would expect. I was worried mainly about places where no copy at all was currently done, but it doesn't seem that there are in fact any of those, and there's always a copy being made, so this isn't as big of a deal. |
|
This one: Will make a copy, which will in reality be moved since it's std::string and the returned "copy" is a rvalue reference from which the final destination object can simply steal the internal pointer to the actual string data. And a function like this: will on any modern compiler end up being 100% equivalant to: So no more copies than the one already in the old code in the initializer list of the Lexer constructor: So no more copies than before. The toLexerStringtype() function call will not exist in the final compiled code. Is there something about my reasoning that you believe to be wrong ? |
|
@gregmarr This surely is a bug, and the current test suite did not find it. I added a test case and committed a change to the |
|
Great. Thanks to @mlutken for finding it. |
|
My thoughts on this:
|
|
We could simply provide the template that has the QT policy class in a separate header so only people needing this would include it. We could reduce the overall number of template parameters by putting more into policy classes or just into this one. Seems that at least the string type should be included here. That way we are back to same number as before. By the way: What other types of string do you support other than std::string ? |
|
Currently, only |
nlohmann
added
the
state: please discuss
|
Any news on this? |
|
@mlutken Did you have the chance to have a look at this? |
|
Inactive. |
nlohmann
removed
state: help needed
|
Hi |
|
But I might have been wrong... |
|
I did not intent to make the changes myself, but I thought that there was an idea for an implementation which did not rely on QT headers and that would not require to much changes. I'm still open for this, but as there were no reaction for nearly 2 months, I'd rather wanted to close the ticket. That said, I'm still happy to support more than |
|
I get that. I understand that you want to compile for non Qt without Qt headers.
|
|
The readme section "Arbitrary types conversions" seems to indicate that any type can be converted to json, so I was under the impression that this meant I could convert QStrings for my strings. I wrote a little code for this: inline void to_json(nlohmann::json& j, const QString& q) {
j = nlohmann::json{q.toStdString()};
}
inline void from_json(const nlohmann::json& j, QString& q) {
q = QString::fromStdString(j.get<std::string>());
}however, it appears this results in an array, not a string, using this sample code: nlohmann::json j{{"extension", QString{".jpg"}}};
std::cout << j.dump() << std::endl;So after glancing through this thread, I can presume QStrings do not work, and the method in the read me is probably only for objects, not any arbitrary type. If this is so, maybe the read me should be updated to be more clear on what it supports. |
|
@kainjow I think your inline void to_json(nlohmann::json& j, const QString& q) {
j = nlohmann::json(q.toStdString());
}(note the parentheses) |
|
@nlohmann that indeed works, which confuses me, since the examples in the read me use brace initializer. |
|
I think the README has a lot of examples where an object or array is created. There, brace initialization makes sense. But something like |
|
There is already a section in the README:
|
|
@nlohmann |
Hi
It would be very nice if one could do:
using json = nlohmann::basic_json < std::map, std::vector, QString >;
And have everything work.
Right now it seems stringstream are used to stringify etc... So it does not quite compile.
Otherwise I am pretty impressed by the way this library works. Having used hson mostly from PHP, where it's very simple, this is clearly the way to go in C++ :)
-Martin
The text was updated successfully, but these errors were encountered: