Skip to content

Reimpl cformat to suit for bytes#2318

Merged
coolreader18 merged 5 commits into
RustPython:masterfrom
qingshi163:bytes-format
Nov 6, 2020
Merged

Reimpl cformat to suit for bytes#2318
coolreader18 merged 5 commits into
RustPython:masterfrom
qingshi163:bytes-format

Conversation

@qingshi163
Copy link
Copy Markdown
Contributor

No description provided.

@qingshi163
Copy link
Copy Markdown
Contributor Author

Fixes #2121

Comment thread vm/src/cformat.rs Outdated

#[derive(Debug, PartialEq)]
pub(crate) struct CFormatBytes {
parts: Vec<(usize, CFormatPart)>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CFormatPart should probably be generic to allow String or Vec<u8>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that is necessary, I am using String as Vec here for bytes. As String stores char as utf-8 so I don't think any cost here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@coolreader18 do you have an example which distinguish bytes and str for the case?

Copy link
Copy Markdown
Member

@coolreader18 coolreader18 Nov 2, 2020

Choose a reason for hiding this comment

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

Well bytes can have non-ascii and still be formatted, while a String wouldn't conserve that:

>>> b'\xaa %b' % b'123'
b'\xaa 123'

Specifically, the u8 as char casting in this PR would map the non-ascii byte \xaa to the utf8 character sequence 'ª' == b'\xc2\xaa'. It's probably better than panicking, but it's lossy when it shouldn't need to be.

Comment thread vm/src/cformat.rs Outdated
Comment thread vm/src/cformat.rs Outdated

#[derive(Debug, PartialEq)]
pub(crate) struct CFormatBytes {
parts: Vec<(usize, CFormatPart)>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@coolreader18 do you have an example which distinguish bytes and str for the case?

Copy link
Copy Markdown
Member

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

Looks great!!

@coolreader18 coolreader18 merged commit 0b9beeb into RustPython:master Nov 6, 2020
@qingshi163 qingshi163 deleted the bytes-format branch November 8, 2020 07:34
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