Skip to content

bpo-33578: Add getstate/setstate for CJK codec #6984

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.

Sign up for GitHub

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

Merged
miss-islington merged 1 commit into from
Nov 1, 2018
Merged

bpo-33578: Add getstate/setstate for CJK codec #6984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions Lib/test/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2957,6 +2957,34 @@ def test_seek_and_tell_with_data(data, min_pos=0):
finally:
StatefulIncrementalDecoder.codecEnabled = 0

def test_multibyte_seek_and_tell(self):
f = self.open(support.TESTFN, "w", encoding="euc_jp")
f.write("AB\n\u3046\u3048\n")
f.close()

f = self.open(support.TESTFN, "r", encoding="euc_jp")
self.assertEqual(f.readline(), "AB\n")
p0 = f.tell()
self.assertEqual(f.readline(), "\u3046\u3048\n")
p1 = f.tell()
f.seek(p0)
self.assertEqual(f.readline(), "\u3046\u3048\n")
self.assertEqual(f.tell(), p1)
f.close()

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

def test_seek_with_encoder_state(self):
f = self.open(support.TESTFN, "w", encoding="euc_jis_2004")
f.write("\u00e6\u0300")
p0 = f.tell()
f.write("\u00e6")
f.seek(p0)
f.write("\u0300")
f.close()

f = self.open(support.TESTFN, "r", encoding="euc_jis_2004")
self.assertEqual(f.readline(), "\u00e6\u0300\u0300")
f.close()

def test_encoded_writes(self):
data = "1234567890"
tests = ("utf-16",
Expand Down
113 changes: 113 additions & 0 deletions Lib/test/test_multibytecodec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,88 @@ def test_stateful_keep_buffer(self):
self.assertRaises(UnicodeEncodeError, encoder.encode, '\u0123')
self.assertEqual(encoder.encode('', True), b'\xa9\xdc')

def test_state_methods_with_buffer_state(self):
# euc_jis_2004 stores state as a buffer of pending bytes
encoder = codecs.getincrementalencoder('euc_jis_2004')()

initial_state = encoder.getstate()
self.assertEqual(encoder.encode('\u00e6\u0300'), b'\xab\xc4')
encoder.setstate(initial_state)
self.assertEqual(encoder.encode('\u00e6\u0300'), b'\xab\xc4')

self.assertEqual(encoder.encode('\u00e6'), b'')
partial_state = encoder.getstate()
self.assertEqual(encoder.encode('\u0300'), b'\xab\xc4')
encoder.setstate(partial_state)
self.assertEqual(encoder.encode('\u0300'), b'\xab\xc4')

def test_state_methods_with_non_buffer_state(self):
# iso2022_jp stores state without using a buffer
encoder = codecs.getincrementalencoder('iso2022_jp')()

self.assertEqual(encoder.encode('z'), b'z')
en_state = encoder.getstate()

self.assertEqual(encoder.encode('\u3042'), b'\x1b\x24\x42\x24\x22')
jp_state = encoder.getstate()
self.assertEqual(encoder.encode('z'), b'\x1b\x28\x42z')

encoder.setstate(jp_state)
self.assertEqual(encoder.encode('\u3042'), b'\x24\x22')

encoder.setstate(en_state)
self.assertEqual(encoder.encode('z'), b'z')

def test_getstate_returns_expected_value(self):
# Note: getstate is implemented such that these state values
# are expected to be the same across all builds of Python,
# regardless of x32/64 bit, endianness and compiler.

# euc_jis_2004 stores state as a buffer of pending bytes
buffer_state_encoder = codecs.getincrementalencoder('euc_jis_2004')()
self.assertEqual(buffer_state_encoder.getstate(), 0)
buffer_state_encoder.encode('\u00e6')
self.assertEqual(buffer_state_encoder.getstate(),
int.from_bytes(
b"\x02"
b"\xc3\xa6"
b"\x00\x00\x00\x00\x00\x00\x00\x00",
'little'))
buffer_state_encoder.encode('\u0300')
self.assertEqual(buffer_state_encoder.getstate(), 0)

# iso2022_jp stores state without using a buffer
non_buffer_state_encoder = codecs.getincrementalencoder('iso2022_jp')()
self.assertEqual(non_buffer_state_encoder.getstate(),
int.from_bytes(
b"\x00"
b"\x42\x42\x00\x00\x00\x00\x00\x00",
'little'))
non_buffer_state_encoder.encode('\u3042')
self.assertEqual(non_buffer_state_encoder.getstate(),
int.from_bytes(
b"\x00"
b"\xc2\x42\x00\x00\x00\x00\x00\x00",
'little'))

def test_setstate_validates_input_size(self):
encoder = codecs.getincrementalencoder('euc_jp')()
pending_size_nine = int.from_bytes(
b"\x09"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00",
'little')
self.assertRaises(UnicodeError, encoder.setstate, pending_size_nine)

def test_setstate_validates_input_bytes(self):
encoder = codecs.getincrementalencoder('euc_jp')()
invalid_utf8 = int.from_bytes(
b"\x01"
b"\xff"
b"\x00\x00\x00\x00\x00\x00\x00\x00",
'little')
self.assertRaises(UnicodeDecodeError, encoder.setstate, invalid_utf8)

def test_issue5640(self):
encoder = codecs.getincrementalencoder('shift-jis')('backslashreplace')
self.assertEqual(encoder.encode('\xff'), b'\\xff')
Expand Down Expand Up @@ -165,6 +247,37 @@ def test_decode_unicode(self):
decoder = codecs.getincrementaldecoder(enc)()
self.assertRaises(TypeError, decoder.decode, "")

def test_state_methods(self):
decoder = codecs.getincrementaldecoder('euc_jp')()

# Decode a complete input sequence
self.assertEqual(decoder.decode(b'\xa4\xa6'), '\u3046')
pending1, _ = decoder.getstate()
self.assertEqual(pending1, b'')

# Decode first half of a partial input sequence
self.assertEqual(decoder.decode(b'\xa4'), '')
pending2, flags2 = decoder.getstate()
self.assertEqual(pending2, b'\xa4')

# Decode second half of a partial input sequence
self.assertEqual(decoder.decode(b'\xa6'), '\u3046')
pending3, _ = decoder.getstate()
self.assertEqual(pending3, b'')

# Jump back and decode second half of partial input sequence again
decoder.setstate((pending2, flags2))
self.assertEqual(decoder.decode(b'\xa6'), '\u3046')
pending4, _ = decoder.getstate()
self.assertEqual(pending4, b'')

def test_setstate_validates_input(self):
decoder = codecs.getincrementaldecoder('euc_jp')()
self.assertRaises(TypeError, decoder.setstate, 123)
self.assertRaises(TypeError, decoder.setstate, ("invalid", 0))
self.assertRaises(TypeError, decoder.setstate, (b"1234", "invalid"))
self.assertRaises(UnicodeError, decoder.setstate, (b"123456789", 0))

class Test_StreamReader(unittest.TestCase):
def test_bug1728403(self):
try:
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1609,6 +1609,7 @@ Nicolas M. Thiéry
James Thomas
Robin Thomas
Brian Thorne
Christopher Thorne
Stephen Thorne
Jeremy Thurgood
Eric Tiedemann
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2018-06-08-23-55-34.bpo-33578.7oSsjG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implement multibyte encoder/decoder state methods
38 changes: 22 additions & 16 deletions Modules/cjkcodecs/_codecs_cn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@
; \
}

/*
* codecs in this file use the first byte of MultibyteCodec_State.c[8]
* to store a 0 or 1 state value
*/
#define CN_STATE_OFFSET 0

/*

@doerwalter doerwalter Oct 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the meaning of this #define?

@libcthorne libcthorne Oct 22, 2018 •
edited
Loading

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 felt c[STATE_OFFSET] was more explicit than c[0], but don't mind inlining it if you prefer. Alternatively, STATE_POSITION may be a better name, as I wanted to get across that only byte 0 is relevant in state->c for the CN codec.

Perhaps even CN_STATE_POSITION?

@doerwalter doerwalter Oct 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, I get it: c[8] is the merged version of the previous union, so it serves two purposes: it's a character array and a (very) small integer. IMHO this (and how it's used) should be documented where MultibyteCodec_State is defined.

@libcthorne libcthorne Oct 22, 2018

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.

If I were to document how I see MultibyteCodec_State now, I would say something along the lines of:

A union that provides 8 bytes of state for multibyte codecs. Codecs
are free to use this how they want. Note: if you need to add a new
member to this union, ensure that its byte order is independent of CPU
endianness so that the return value of `getstate` doesn't differ
between little and big endian CPUs.

Should I add this as a comment next to the union?

@doerwalter doerwalter Oct 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. However MultibyteCodec_State no longer is a union, "if you need to add a new member to this union" should be phrased differently.

And STATE_OFFSET would have a comment like: "Codecs that have to store a small integer in MultibyteCodec_State store it at the following offset".

And yes, multibytecodec.h is a little light on documentation, but that's probably another patch.

@libcthorne libcthorne Oct 23, 2018

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've added comments next to STATE_OFFSET (now CN_STATE_OFFSET for clarity) and MultibyteCodec_State. Please let me know if the explanations make sense.

@doerwalter doerwalter Oct 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, but I'd like @methane to make the final decision about merging this PR.

* GB2312 codec
*/
Expand Down Expand Up @@ -329,15 +335,15 @@ DECODER(gb18030)

ENCODER_INIT(hz)
{
state->i = 0;
state->c[CN_STATE_OFFSET] = 0;
return 0;
}

ENCODER_RESET(hz)
{
if (state->i != 0) {
if (state->c[CN_STATE_OFFSET] != 0) {
WRITEBYTE2('~', '}');
state->i = 0;
state->c[CN_STATE_OFFSET] = 0;
NEXT_OUT(2);
}
return 0;
Expand All @@ -350,10 +356,10 @@ ENCODER(hz)
DBCHAR code;

if (c < 0x80) {
if (state->i) {
if (state->c[CN_STATE_OFFSET]) {
WRITEBYTE2('~', '}');
NEXT_OUT(2);
state->i = 0;
state->c[CN_STATE_OFFSET] = 0;
}
WRITEBYTE1((unsigned char)c);
NEXT(1, 1);
Expand All @@ -375,10 +381,10 @@ ENCODER(hz)
if (code & 0x8000) /* MSB set: GBK */
return 1;

if (state->i == 0) {
if (state->c[CN_STATE_OFFSET] == 0) {
WRITEBYTE4('~', '{', code >> 8, code & 0xff);
NEXT(1, 4);
state->i = 1;
state->c[CN_STATE_OFFSET] = 1;
}
else {
WRITEBYTE2(code >> 8, code & 0xff);
Expand All @@ -391,13 +397,13 @@ ENCODER(hz)

DECODER_INIT(hz)
{
state->i = 0;
state->c[CN_STATE_OFFSET] = 0;
return 0;
}

DECODER_RESET(hz)
{
state->i = 0;
state->c[CN_STATE_OFFSET] = 0;
return 0;
}

Expand All @@ -411,14 +417,14 @@ DECODER(hz)
unsigned char c2 = INBYTE2;

REQUIRE_INBUF(2);
if (c2 == '~' && state->i == 0)
if (c2 == '~' && state->c[CN_STATE_OFFSET] == 0)
OUTCHAR('~');
else if (c2 == '{' && state->i == 0)
state->i = 1; /* set GB */
else if (c2 == '\n' && state->i == 0)
else if (c2 == '{' && state->c[CN_STATE_OFFSET] == 0)
state->c[CN_STATE_OFFSET] = 1; /* set GB */
else if (c2 == '\n' && state->c[CN_STATE_OFFSET] == 0)
; /* line-continuation */
else if (c2 == '}' && state->i == 1)
state->i = 0; /* set ASCII */
else if (c2 == '}' && state->c[CN_STATE_OFFSET] == 1)
state->c[CN_STATE_OFFSET] = 0; /* set ASCII */
else
return 1;
NEXT_IN(2);
Expand All @@ -428,7 +434,7 @@ DECODER(hz)
if (c & 0x80)
return 1;

if (state->i == 0) { /* ASCII mode */
if (state->c[CN_STATE_OFFSET] == 0) { /* ASCII mode */
OUTCHAR(c);
NEXT_IN(1);
}
Expand Down
90 changes: 89 additions & 1 deletion Modules/cjkcodecs/clinic/multibytecodec.c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,50 @@ _multibytecodec_MultibyteIncrementalEncoder_encode(MultibyteIncrementalEncoderOb
return return_value;
}

PyDoc_STRVAR(_multibytecodec_MultibyteIncrementalEncoder_getstate__doc__,
"getstate($self, /)\n"
"--\n"
"\n");

#define _MULTIBYTECODEC_MULTIBYTEINCREMENTALENCODER_GETSTATE_METHODDEF \
{"getstate", (PyCFunction)_multibytecodec_MultibyteIncrementalEncoder_getstate, METH_NOARGS, _multibytecodec_MultibyteIncrementalEncoder_getstate__doc__},

static PyObject *
_multibytecodec_MultibyteIncrementalEncoder_getstate_impl(MultibyteIncrementalEncoderObject *self);

static PyObject *
_multibytecodec_MultibyteIncrementalEncoder_getstate(MultibyteIncrementalEncoderObject *self, PyObject *Py_UNUSED(ignored))
{
return _multibytecodec_MultibyteIncrementalEncoder_getstate_impl(self);
}

PyDoc_STRVAR(_multibytecodec_MultibyteIncrementalEncoder_setstate__doc__,
"setstate($self, state, /)\n"
"--\n"
"\n");

#define _MULTIBYTECODEC_MULTIBYTEINCREMENTALENCODER_SETSTATE_METHODDEF \
{"setstate", (PyCFunction)_multibytecodec_MultibyteIncrementalEncoder_setstate, METH_O, _multibytecodec_MultibyteIncrementalEncoder_setstate__doc__},

static PyObject *
_multibytecodec_MultibyteIncrementalEncoder_setstate_impl(MultibyteIncrementalEncoderObject *self,
PyLongObject *statelong);

static PyObject *
_multibytecodec_MultibyteIncrementalEncoder_setstate(MultibyteIncrementalEncoderObject *self, PyObject *arg)
{
PyObject *return_value = NULL;
PyLongObject *statelong;

if (!PyArg_Parse(arg, "O!:setstate", &PyLong_Type, &statelong)) {
goto exit;
}
return_value = _multibytecodec_MultibyteIncrementalEncoder_setstate_impl(self, statelong);

exit:
return return_value;
}

PyDoc_STRVAR(_multibytecodec_MultibyteIncrementalEncoder_reset__doc__,
"reset($self, /)\n"
"--\n"
Expand Down Expand Up @@ -169,6 +213,50 @@ _multibytecodec_MultibyteIncrementalDecoder_decode(MultibyteIncrementalDecoderOb
return return_value;
}

PyDoc_STRVAR(_multibytecodec_MultibyteIncrementalDecoder_getstate__doc__,
"getstate($self, /)\n"
"--\n"
"\n");

#define _MULTIBYTECODEC_MULTIBYTEINCREMENTALDECODER_GETSTATE_METHODDEF \
{"getstate", (PyCFunction)_multibytecodec_MultibyteIncrementalDecoder_getstate, METH_NOARGS, _multibytecodec_MultibyteIncrementalDecoder_getstate__doc__},

static PyObject *
_multibytecodec_MultibyteIncrementalDecoder_getstate_impl(MultibyteIncrementalDecoderObject *self);

static PyObject *
_multibytecodec_MultibyteIncrementalDecoder_getstate(MultibyteIncrementalDecoderObject *self, PyObject *Py_UNUSED(ignored))
{
return _multibytecodec_MultibyteIncrementalDecoder_getstate_impl(self);
}

PyDoc_STRVAR(_multibytecodec_MultibyteIncrementalDecoder_setstate__doc__,
"setstate($self, state, /)\n"
"--\n"
"\n");

#define _MULTIBYTECODEC_MULTIBYTEINCREMENTALDECODER_SETSTATE_METHODDEF \
{"setstate", (PyCFunction)_multibytecodec_MultibyteIncrementalDecoder_setstate, METH_O, _multibytecodec_MultibyteIncrementalDecoder_setstate__doc__},

static PyObject *
_multibytecodec_MultibyteIncrementalDecoder_setstate_impl(MultibyteIncrementalDecoderObject *self,
PyObject *state);

static PyObject *
_multibytecodec_MultibyteIncrementalDecoder_setstate(MultibyteIncrementalDecoderObject *self, PyObject *arg)
{
PyObject *return_value = NULL;
PyObject *state;

if (!PyArg_Parse(arg, "O!:setstate", &PyTuple_Type, &state)) {
goto exit;
}
return_value = _multibytecodec_MultibyteIncrementalDecoder_setstate_impl(self, state);

exit:
return return_value;
}

PyDoc_STRVAR(_multibytecodec_MultibyteIncrementalDecoder_reset__doc__,
"reset($self, /)\n"
"--\n"
Expand Down Expand Up @@ -330,4 +418,4 @@ PyDoc_STRVAR(_multibytecodec___create_codec__doc__,

#define _MULTIBYTECODEC___CREATE_CODEC_METHODDEF \
{"__create_codec", (PyCFunction)_multibytecodec___create_codec, METH_O, _multibytecodec___create_codec__doc__},
/*[clinic end generated code: output=680f59f4cfe63c25 input=a9049054013a1b77]*/
/*[clinic end generated code: output=2fa0a38494716b97 input=a9049054013a1b77]*/
Loading