Added emitUTF8 setting. #1045
Added emitUTF8 setting. #1045
Conversation
src/lib_json/json_writer.cpp
Outdated
| @@ -267,7 +267,7 @@ static String toHex16Bit(unsigned int x) { | |||
| return result; | |||
| } | |||
|
|
|||
| static String valueToQuotedStringN(const char* value, unsigned length) { | |||
| static String valueToQuotedStringN(const char* value, unsigned length, bool emitUTF8 = false ) { | |||
There was a problem hiding this comment.
It's better to overload it instead of rewriting it, I guess?
etc, use it outside the method
if (_emitUTF8) {
....
} else {
valueToQuotedStringN(value, length);
}
There was a problem hiding this comment.
I want to use the escaping of the tabs, newlines etc. in the first part of the function and then either write the UTF-8 characters or unicode escape them in the second part. Perhaps I should split the function into those separate pieces? What's your view?
There was a problem hiding this comment.
I actually prefer it as is. There's nothing wrong with the default argument usage here, especially since most of the downsides of default arguments don't apply to static methods.
There was a problem hiding this comment.
I'm glad that you're all considering binary compatibility. Thank you.
Yes, internal "statics" are hidden from the public API, so either way is fine with me.
This change is a nice idea.
|
In addition, it's better to write some testcases to test this change. |
I'll add some tests this evening. |
src/lib_json/json_writer.cpp
Outdated
| @@ -267,7 +267,7 @@ static String toHex16Bit(unsigned int x) { | |||
| return result; | |||
| } | |||
|
|
|||
| static String valueToQuotedStringN(const char* value, unsigned length) { | |||
| static String valueToQuotedStringN(const char* value, unsigned length, bool emitUTF8 = false ) { | |||
There was a problem hiding this comment.
I actually prefer it as is. There's nothing wrong with the default argument usage here, especially since most of the downsides of default arguments don't apply to static methods.
|
Great! Thanks！ |
|
Good for it. |
|
why didn't we add a description to |
|
Thanks so much for adding in this functionality, it's desperately needed. But I'm still baffled why it's opt-in?
UTF-8 encoding absolutely should be the defaut. |
This PR adds a new setting 'emitUTF8' which avoids unicode escaping the emitted JSON.