Add support for \x?? and improve escapes #84

torkleyy · 2018-01-21T18:36:58Z

~~This PR seems to contain a regression.~~

Fixes #82

torkleyy · 2018-01-21T18:53:19Z

This was a stupid mistake, fixed it now :)

This is ready for review.

torkleyy · 2018-01-21T18:53:41Z

r? @pyfisch

pyfisch · 2018-01-21T19:25:47Z

Thanks. How about using char::escape_debug instead for both chars and strings.
There is also a u32::from_str_radix to parse hexadecimal numbers.
Rust itself only support very few special escapes and RON should IMHO not invent new ones (or copy them from JSON).

torkleyy · 2018-01-21T20:13:39Z

That's a good idea, I'll try it out tomorrow.

torkleyy · 2018-02-10T19:48:23Z

@pyfisch Is there a reason you suggested escape_debug over e.g. escape_default?

pyfisch · 2018-02-10T21:14:35Z

@torkleyy That I like to be able to read Unicode characters. (escape_unicode escapes just everything)

torkleyy · 2018-02-10T21:22:46Z

Ah, thanks for the explanation :)

kvark · 2018-02-11T02:58:28Z

bors delegate=pyfisch

bors · 2018-02-11T02:58:29Z

✌️ pyfisch can now approve this pull request

torkleyy · 2018-02-11T08:30:21Z

I finally finished this :) r? @pyfisch @kvark

Will squash once approved.

pyfisch

Great work! Added a few comments.

pyfisch · 2018-02-11T08:59:34Z

src/parse.rs

@@ -94,7 +95,7 @@ impl<'a> Bytes<'a> {
            let c = self.eat_byte()?;

            if c != b'\\' && c != b'\'' {
-                return self.err(ParseError::InvalidEscape);
+                return self.err(ParseError::InvalidEscape("TODO"));


Mind to resolve the TODO?

pyfisch · 2018-02-11T09:01:17Z

src/parse.rs

+        Ok(n)
+    }
+
+    /*


Remove the commented out code.

pyfisch · 2018-02-11T09:04:35Z

src/ser/mod.rs

+
+    fn serialize_escaped_str(&mut self, value: &str) {
+        self.output += "\"";
+        self.output += &value.chars().flat_map(|c| c.escape_default()).collect::<String>();


I would avoid the string allocation and immediately push the characters to the output.

Err right, I was trying it out and forgot about it then. Thanks!

pyfisch · 2018-02-11T09:10:31Z

tests/escape.rs

+    let s: String = (1..128).into_iter().flat_map(from_u32).collect();
+
+    check_same(s);
+}


Some test with non-ascii characters would be nice.

For example ♠ and ß. (They should not be escaped)

pyfisch · 2018-02-11T09:12:35Z

src/parse.rs

-                        }
+                if bytes == 0 {
+                    return self.err(ParseError::InvalidEscape(
+                        "Null character (\\u{0}) not allowed",


Why is the null character not allowed? "foo\0bar" is a valid string in Rust.

I don't think we should allow it in RON though, since that would make usage in e.g. C really difficult.

pyfisch · 2018-02-11T09:14:10Z

src/parse.rs


    fn parse_str_escape(&mut self, store: &mut Vec<u8>) -> Result<()> {
        use std::iter::repeat;

        match self.eat_byte()? {
+            b'\'' => store.push(b'\''),
            b'"' => store.push(b'"'),
            b'\\' => store.push(b'\\'),
            b'b' => store.push(b'\x08'),


Some of these escapes are no standard Rust escapes.

torkleyy · 2018-02-11T10:32:29Z

Okay, addressed all comments except one (about \0).

pyfisch · 2018-02-11T11:57:34Z

I don't like the current handling of NUL. Currently the string "foo\0bar" will successfully be serialized. But it will fail deserialization.

My assumption is that I can serialize any Rust data type with RON and deserialize it again and get an equivalent representation. Special casing strings with a NUL character breaks this assumption.

torkleyy · 2018-02-11T12:03:54Z

You're right that serialization and deserialization should allow the same things. If we decide to exclude \0 I will add a check to the serialization.

@kvark @ozkriff @Xaeroxe What do you think about this?

My main concern is that adding support for \0 would make using RON in e.g. C really difficult. Some parsers for e.g. JSON have to use special hacks and callbacks to make \0 inside a string work.

pyfisch · 2018-02-11T12:11:39Z

My primary concern is that if we exclude \0 somewhere in Servo a string with a \0 will occur and on serialization RON panics. In this case servo will probably be forced to use a different format without this quirk. (I think the current use cases for debugging in Servo and WebRender do not allow strings with \0 but it is better to be safe)

torkleyy · 2018-02-11T12:13:52Z

It doesn't panic, just gives you an error. And really, it doesn't make any sense to include \0 in a string. If a format supports that it's a bug, not a feature.

kvark · 2018-02-11T12:39:58Z

C interop and serialization are somewhat orthogonal. I could see a problem with other languages implementing RON support and treating this differently. For that we should have NULL handling defined as implementation dependent. There are cases in OpenGL/Vulkan spec where they do the same: it's not undefined behavior, and it's not standard, it's a defined per implementation behavior.

TL;DR: we should support \0 in this implementation.

pyfisch

Perfect!

pyfisch · 2018-02-13T10:52:45Z

bors delegate+

Merge it after squashing.

bors · 2018-02-13T10:52:46Z

✌️ torkleyy can now approve this pull request

Add support for "\x??" Add more tests, fix bug Finish string escapes Add non ascii test, fix nits Remove non-standard escapes Implement char escapes Allow NUL

torkleyy · 2018-02-13T15:26:28Z

bors r=pyfisch

84: Add support for \x?? and improve escapes r=pyfisch a=torkleyy ~~**This PR seems to contain a regression.**~~ Fixes #82

bors · 2018-02-13T15:29:22Z

Build succeeded

continuous-integration/travis-ci/push

torkleyy added the working label Jan 21, 2018

torkleyy force-pushed the x branch from b9e19ee to 8019b7a Compare January 21, 2018 18:53

torkleyy removed the working label Jan 21, 2018

pyfisch suggested changes Feb 11, 2018

View reviewed changes

pyfisch approved these changes Feb 13, 2018

View reviewed changes

Improve unicode support, add \x?? and \u{?}

2e19ca2

Add support for "\x??" Add more tests, fix bug Finish string escapes Add non ascii test, fix nits Remove non-standard escapes Implement char escapes Allow NUL

torkleyy force-pushed the x branch from 87f3bd7 to 2e19ca2 Compare February 13, 2018 15:25

bors bot added a commit that referenced this pull request Feb 13, 2018

Merge #84

1d4c942

84: Add support for \x?? and improve escapes r=pyfisch a=torkleyy ~~**This PR seems to contain a regression.**~~ Fixes #82

bors bot merged commit 2e19ca2 into ron-rs:master Feb 13, 2018

torkleyy deleted the x branch February 13, 2018 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for \x?? and improve escapes #84

Add support for \x?? and improve escapes #84

torkleyy commented Jan 21, 2018 •

edited

Loading

torkleyy commented Jan 21, 2018

torkleyy commented Jan 21, 2018

pyfisch commented Jan 21, 2018

torkleyy commented Jan 21, 2018 via email

torkleyy commented Feb 10, 2018

pyfisch commented Feb 10, 2018 •

edited

Loading

torkleyy commented Feb 10, 2018 via email

kvark commented Feb 11, 2018

bors bot commented Feb 11, 2018

torkleyy commented Feb 11, 2018 •

edited

Loading

pyfisch left a comment

pyfisch Feb 11, 2018

pyfisch Feb 11, 2018

pyfisch Feb 11, 2018

torkleyy Feb 11, 2018

pyfisch Feb 11, 2018

pyfisch Feb 11, 2018

torkleyy Feb 11, 2018

pyfisch Feb 11, 2018

torkleyy commented Feb 11, 2018

pyfisch commented Feb 11, 2018

torkleyy commented Feb 11, 2018

pyfisch commented Feb 11, 2018

torkleyy commented Feb 11, 2018

kvark commented Feb 11, 2018

pyfisch left a comment

pyfisch commented Feb 13, 2018

bors bot commented Feb 13, 2018

torkleyy commented Feb 13, 2018

bors bot commented Feb 13, 2018

+                      Ok(n)
+                  }
+                  /*

Add support for \x?? and improve escapes #84

Add support for \x?? and improve escapes #84

Conversation

torkleyy commented Jan 21, 2018 • edited Loading

torkleyy commented Jan 21, 2018

torkleyy commented Jan 21, 2018

pyfisch commented Jan 21, 2018

torkleyy commented Jan 21, 2018 via email

torkleyy commented Feb 10, 2018

pyfisch commented Feb 10, 2018 • edited Loading

torkleyy commented Feb 10, 2018 via email

kvark commented Feb 11, 2018

bors bot commented Feb 11, 2018

torkleyy commented Feb 11, 2018 • edited Loading

pyfisch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkleyy commented Feb 11, 2018

pyfisch commented Feb 11, 2018

torkleyy commented Feb 11, 2018

pyfisch commented Feb 11, 2018

torkleyy commented Feb 11, 2018

kvark commented Feb 11, 2018

pyfisch left a comment

Choose a reason for hiding this comment

pyfisch commented Feb 13, 2018

bors bot commented Feb 13, 2018

torkleyy commented Feb 13, 2018

bors bot commented Feb 13, 2018

Build succeeded

torkleyy commented Jan 21, 2018 •

edited

Loading

pyfisch commented Feb 10, 2018 •

edited

Loading

torkleyy commented Feb 11, 2018 •

edited

Loading