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

UTF-8 support for JSON-RPC #2740

Merged
merged 2 commits into from
Oct 22, 2013
Merged

Conversation

constantined
Copy link
Contributor

No description provided.

@laanwj
Copy link
Member

laanwj commented Jun 5, 2013

I'm all for supporting UTF-8, but raw utf8 is non-standard JSON and will likely break compatibility with some parsers. Escaping using \uNNNN is the norm. I'm not convinced we should enable it:

raw_utf8 = 0x02, // This prevents non-printable characters from being escapted using "\uNNNN" notation.
                 // Note, this is an extension to the JSON standard. It disables the escaping of
                 // non-printable characters allowing UTF-8 sequences held in 8 bit char strings
                 // to pass through unaltered.

@luke-jr
Copy link
Member

luke-jr commented Jun 5, 2013

@laanwj How is it non-standard? The specification seems to say raw UTF-8 is standard and parsers must handle it...

@constantined
Copy link
Contributor Author

JSON Spirit without raw_utf8 escapes every byte as UTF-8 character. For example, U+0432 will be escaped as \u00D0\u00B2. raw_utf8 is solution.

@laanwj
Copy link
Member

laanwj commented Jun 5, 2013

Well the help text said it is non-standard, and I was going on that, but according to http://www.ietf.org/rfc/rfc4627.txt?number=4627

All Unicode characters may be placed within the
quotation marks except for the characters that must be escaped:
quotation mark, reverse solidus, and the control characters (U+0000
through U+001F).

So, as long as it still escapes those characters it is OK.

@constantined
Copy link
Contributor Author

@jgarzik commented on 370df18
"now builds on unix" tells us nothing about the commit: why it was needed -- because it builds on "unix" here, what needed changing etc. Suggested commit message:

makefile.unix: link with boost_chrono to fix build on [your platform]

Sorry, it was my own issue. Solved by upgrading boost libs from v1.52 to v1.53.

@fanquake
Copy link
Member

Changelog for 4.04 to 4.06

Version 4.04, 8 January 2011
Added the raw_utf8 feature to the write functions
Added a << std::dec before writing to a stream to ensure that integer variables appear in base 10 notation
Added additional .h files to CMakeLists.txt
Output doubles using precision of 17 instead of 16
Added option to remove trailing zeros when outputting doubles
Ensures that write functions return the state of a given IO stream to its original state on completion

Version 4.05, 12 September 2011
Added a new output option for single line arrays
Fixed non-standard zero digits following decimal point with "remove trailing zeros" output option
Reduced build times with new JSON_SPIRIT_VALUE_ENABLED type #defines
Added a constructor for variant types
Added a constructor for container iterators

Version 4.06, 14 May 2013
Added support for Javascript type comments, i.e. // and /* */
Added json_spirit_writer_options.h to CMake install
Improved error message on attempt to extract the wrong type of data from a value
Correct bug that reduced write performance
Added always_escape_nonascii writer flag

http://www.codeproject.com/Articles/20027/JSON-Spirit-A-C-JSON-Parser-Generator-Implemented#hist

@Diapolo
Copy link

Diapolo commented Jun 11, 2013

Any reason not to update?

@jgarzik
Copy link
Contributor

jgarzik commented Jul 22, 2013

I'd like to see json_spirit update separate from any UTF8 fixes. i.e. two commits: update json spirit, then switch raw_utf8

@Diapolo
Copy link

Diapolo commented Jul 22, 2013

@jgarzik Seems like a good thing to do.

Also I hope the new json_spirit version fixes a compiler warning I get:

\src\json\json_spirit_writer_template.h:31: Warnung:typedef 'Char_type' locally defined but not used [-Wunused-local-typedefs]
         typedef typename String_type::value_type Char_type;
                                                  ^

@constantined
Copy link
Contributor Author

@jgarzik Done.
@Diapolo Can't reproduce this warning on master branch. gcc-4.7.3, GNU/Linux.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/2ecb7555a9df1e843fd25f588819e4ca1d94b266 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@Diapolo
Copy link

Diapolo commented Jul 23, 2013

@constantined I also did not have that error with MinGW < 4.8.1, but perhaps latest json_spirit fixes this anyway :).

@jgarzik
Copy link
Contributor

jgarzik commented Aug 25, 2013

Looks pretty good. Made one inline comment.

Do Qt guys agree that RPC console should pretty print?

@Diapolo
Copy link

Diapolo commented Aug 29, 2013

Did a quick web search, but perhaps you can tell me what pretty-print means for RPC console or perhaps give a formated example?

@laanwj
Copy link
Member

laanwj commented Aug 30, 2013

I tried this out and didn't see any change to the JSON as printed in the console before/after this patch. So it's ACK by me.

@wtogami
Copy link
Contributor

wtogami commented Sep 8, 2013

If this happens, please be sure that #2980 is fixed.

@laanwj
Copy link
Member

laanwj commented Oct 21, 2013

Is there anything blocking this?

@gavinandresen
Copy link
Contributor

Works for me.

gavinandresen added a commit that referenced this pull request Oct 22, 2013
@gavinandresen gavinandresen merged commit 125bdea into bitcoin:master Oct 22, 2013
@jgarzik
Copy link
Contributor

jgarzik commented Oct 22, 2013

Had to revert this -- it broke JSON-RPC values. See #3126

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Apr 5, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants