GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Implement __reduce__ and __reduce_ex__ for array by qingshi163 · Pull Request #3064 · RustPython/RustPython

Conversation

qingshi163 commented Sep 15, 2021

Copy link

Copy Markdown

Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

qingshi163 commented

Sep 15, 2021

View reviewed changes

Comment thread

vm/src/stdlib/array.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

qingshi163 commented

Sep 15, 2021

View reviewed changes

Comment thread

vm/src/stdlib/array.rs Outdated

Comment on lines +564 to +576

		vm.ctx.new_str(
		char::from_u32(self.0 as u32)
		.unwrap_or_default()
		.to_string(),
		)

qingshi163 Sep 15, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

is the convert safe?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone Sep 15, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

WideChar can contain invalid utf-8 character like surrogate. So I don't think so. To be safe, It must be decoded.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

qingshi163 Sep 15, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Are we assuming the data in the array is decoded?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone Sep 15, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I am not sure. I guess this is corresponding to WChar unicode object in CPython - which is deprecated same as this type array.
Even if it is regarded as a decoded string, because this is array, it still can contain surrogate character, which is not a valid utf8 string.
So my last comment was wrong. Regardless it is decoded or not, it can contains invalid utf8 character.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone requested changes

Sep 15, 2021

View reviewed changes

Comment thread

vm/src/builtins/float.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

vm/src/stdlib/array.rs Outdated

Comment on lines +564 to +576

		vm.ctx.new_str(
		char::from_u32(self.0 as u32)
		.unwrap_or_default()
		.to_string(),
		)

youknowone Sep 15, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

WideChar can contain invalid utf-8 character like surrogate. So I don't think so. To be safe, It must be decoded.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

qingshi163 force-pushed the array-pickle branch 2 times, most recently from f6dd539 to c289397 Compare

September 16, 2021 13:37

qingshi163 commented Sep 16, 2021

Copy link

Copy Markdown

Contributor Author

@youknowone Can you check what went wrong in windows tests and fix it?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone reviewed

Sep 16, 2021

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I am really sorry. I don't have an accessible windows machine for now and until early october.

can anyone help this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

extra_tests/snippets/stdlib_array.py Outdated

		test_str = '🌉abc🌐def🌉🌐'
		u = array('u', test_str)
		assert u.__reduce_ex__(1)[1][1] == list(test_str)
		assert str(loads(dumps(u, 1))) == f"array('u', '{test_str}')"

youknowone Sep 16, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	assert str(loads(dumps(u, 1))) == f"array('u', '{test_str}')"
	assert str(loads(dumps(u, 1))) == f"array('u', '{test_str}')", str(loads(dumps(u, 1)))

then it will shows the value on assertion failure

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

fanninpm Sep 16, 2021

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

(jedi mind trick wave) That was not the line you're looking for…

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

fanninpm commented Sep 16, 2021

Copy link

Copy Markdown

Contributor

@coolreader18 might be able to diagnose the Windows problem

coolreader18 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone commented Oct 2, 2021

Copy link

Copy Markdown

Member

@qingshi163 could you rebase this PR? I will check windows problem during this week

qingshi163 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

qingshi163 force-pushed the array-pickle branch from c289397 to a0db9f7 Compare

October 2, 2021 16:04

youknowone reviewed

Oct 4, 2021

View reviewed changes

Comment thread

stdlib/src/array.rs

Comment on lines +1146 to +1149

		let s = Self::_wchar_bytes_to_string(array.get_bytes(), array.itemsize(), vm)?;
		s.chars().map(\|x\| x.into_pyobject(vm)).collect()

youknowone Oct 4, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

In windows, I tried this code with CPython:

test_str = '🌉abc🌐def🌉🌐'
u = array('u', test_str)
print(u.__reduce_ex__(1))

then

  File ".\extra_tests\snippets\stdlib_array.py", line 104, in <module>
    assert u.__reduce_ex__(1)[1][1] == list(test_str), (u.__reduce_ex__(1)[1][1], list(test_str))
AssertionError: (['\ud83c', '\udf09', 'a', 'b', 'c', '\ud83c', '\udf10', 'd', 'e', 'f', '\ud83c', '\udf09', '\ud83c', '\udf10'], ['🌉', 'a', 'b', 'c', '🌐', 'd', 'e', 'f', '🌉', '🌐'])

The values are wchar encoded in CPython but doesn't look like that here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

qingshi163 Oct 6, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do you mean CPython did not treat it as same in linux macos?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone Oct 6, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, it seems. I think that is not exactly the platform-specific variant but sizeof(wchar_t) is different by platforms. Looks very fragile in point of view of compatibility, no wonder why it is deprecated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone Oct 6, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't mind if you want to just skip testing for 16bit character environments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

qingshi163 force-pushed the array-pickle branch 2 times, most recently from ee6fa13 to be2bf61 Compare

October 7, 2021 13:41

youknowone approved these changes

Oct 7, 2021

View reviewed changes

qingshi163 added 4 commits

October 10, 2021 14:43


          Impl __reduce__ for array

ff0adc1


          Impl __reduce_ex__ with _array_reconstructor

de47cd0


          Fix unicode array pickling

cbd7c59


          remove test that fail on CPython

ea69dc5

qingshi163 force-pushed the array-pickle branch from 9af4998 to ea69dc5 Compare

October 10, 2021 12:48

qingshi163 requested a review from youknowone

October 10, 2021 19:47

youknowone approved these changes

Oct 11, 2021

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you for the long time effort!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone merged commit b986e6b into RustPython:main

Oct 11, 2021

fanninpm mentioned this pull request

Jul 15, 2022

replace array.__reduce__ to array.__reduce_ex__ #3876

Closed

qingshi163 deleted the array-pickle branch

August 16, 2022 07:26

Reviewers

DimitrisJim

DimitrisJim left review comments

fanninpm

fanninpm left review comments

youknowone

youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.