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

Implement support for \uxxxx escape sequences. Fixes #304 #791

Closed
wants to merge 7 commits into from

Conversation

trilader
Copy link

@trilader trilader commented Aug 8, 2018

Currently there are no unit tests for this, as I've got no idea which file would be appropriate for this kind of test case.

@coveralls
Copy link

coveralls commented Aug 8, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 81f9026 on trilader:master into fa1a40a on bblanchon:master.

@bblanchon
Copy link
Owner

Hi @trilader,

Thank you very much for this Pull Request.

The important thing concerning issue #304 is the code size.
Very few people are interested in this feature, so it's not worth wasting the rare bytes of the UNO for code that nobody uses.

With this PR, you only added 224 bytes on ATmega328.
This is very good but it's still about 8% of the size of the parser.

On the one hand, I don't like the idea of increasing the size by 8% for only 0.001% of the user.
But on the other hand, that would make ArduinoJson compliant with the specification.
So maybe it's worth it.
At least, if it's not something for v5, it's definitely something for v6, where the increase in size won't come by surprise.

Anyway, we first need to be sure that this code works.
Please add some unit tests in test/JsonBuffer/parse.cpp

Also, please send a link (or two) to the documentation that you used as a reference, so I can double check.

Thanks.

Regards,
Benoit.

This saves 34 bytes in JsonParserExample on ATMega328
This saves 18 bytes in JsonParserExample on ATMega328
This saves 10 bytes in JsonParserExample on ATMega328
@trilader
Copy link
Author

I've updated the code and managed to decrease the size impact by about 27% (62 bytes, tested with the JsonParserExample for Arduino Uno)

Regarding the references used:
As a encoding format reference the Linux UTF-8 man page was used (online version).
As the JSON standard only allows four digits in \u escape sequences only encodings up to three bytes are implemented.

The code only produces proper UTF-8 for codepoints < 0x10000, codepoints above that would be encoded as two escapes in JSON using surrogate UTF-16 code units. In this case the implementation will produce CESU-8.

@bblanchon
Copy link
Owner

Hi @trilader,

When I compare the master branch to this one, there is a 182 bytes increase in the size of JsonParserExample.

It's still very good and it's a significant improvement compared to the first version.
I'll check the code, as soon as I get home.

I've been thinking about this issue, and I think the solution to provide this new feature without risking to break existing code would be to make it optional.
We could easily wrap the new code with an #if ARDUINOJSON_ENABLE_SURROGATE and make the default value 1 on PC, but 0 on embedded systems.
There are already some options like that in Configuration.hpp.

Anyway, thank you for this amazing job.

Regards,
Benoit

@bblanchon
Copy link
Owner

Hi Daniel,

I manually integrated the changes in 7050ef6.
It's an opt-in feature; you need to define ARDUINOJSON_DECODE_UNICODE to 1 to use it.
It will be released in version 6.9.

Thank you very much for your work, and please apologize for the long delay.

Regards,
Benoit

@bblanchon bblanchon closed this Feb 15, 2019
Repository owner locked and limited conversation to collaborators Mar 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants