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

closes #12967 fix [en|de]coding of HashMap<K,V> where K is a numeric type #12973

Closed
wants to merge 1 commit into from

Conversation

olsonjeffery
Copy link
Contributor

Details are in #12967.

This is just w/i the serialize::json module. If you have feedback for how to improve this impl, I'm all ears. This is to work around a blocking issue I've encountered with an application I'm working on.

let buf = buf.unwrap();
let out = from_utf8(buf).unwrap();
let needs_wrapping = out.char_at(0) != '"' &&
out.char_at(out.len() - 1) != '"';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail if out has a multibyte trailing character. It should be one of

out.char_at_reverse(out.len());
// or
out.as_bytes()[out.len() - 1] != '"' as u8;

(The last is valid because all 7-bit ASCII bytes in a UTF-8 string represent that ASCII value.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be correct because if there are multibyte characters, it must start with a " and end with a ". That is, it's going to fail anyway because if you have a multibyte character there, it's invalid JSON.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, hm; I guess so. (It's worth having a comment about why we can just wilfully fly in the face of unicode.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises the larger point that our JSON support is really fragile,
though.

On Mon, Mar 17, 2014 at 9:24 AM, Huon Wilson notifications@git.luolix.topwrote:

In src/libserialize/json.rs:

     if idx != 0 { try!(write!(self.wr, ",")) }
  •    f(self)
    
  •    // ref #12967, make sure to wrap a key in double quotes,
    
  •    // in the event that its of a type that omits them (eg numbers)
    
  •    let mut buf = MemWriter::new();
    
  •    let mut check_encoder = Encoder::new(&mut buf);
    
  •    f(&mut check_encoder);
    
  •    let buf = buf.unwrap();
    
  •    let out = from_utf8(buf).unwrap();
    
  •    let needs_wrapping = out.char_at(0) != '"' &&
    
  •        out.char_at(out.len() - 1) != '"';
    

Oh, hm; I guess so. (It's worth having a comment about why we can just
wilfully fly in the face of unicode.)


Reply to this email directly or view it on GitHubhttps://github.com//pull/12973/files#r10655371
.

http://octayn.net/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmr you know I agree but, as I said in reply to someone making the same point in IRC last night: let not the perfect by the enemy of the good.

This is a specific fix to an issue I encountered in the wild and counts as progress. By all means let us open up a follow-on issue to address this, but let's have that discussion somewhere else and not in this PR, whose specific issue is blocking me :P

@huonw
Copy link
Member

huonw commented Mar 17, 2014

r=me with a squash (and working code ;P ).

… numeric type

serialize: ref rust-lang#12697 minor adj. to last char check + prettyencode test
@bors bors closed this in 87e72c3 Mar 19, 2014
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 25, 2024
make [`or_fun_call`] recursive.

fixes: rust-lang#12973

---

changelog: make [`or_fun_call`] recursive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants