-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-32072: Fix issues with binary plists. #4455
bpo-32072: Fix issues with binary plists. #4455
Conversation
* Fixed saving bytearrays. * Identical objects will be saved only once. * Equal references will be load as identical objects. * Added support for saving and loading recursive data structures.
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.
Looks good to me.
I have two small comments, but the code is fine even if those are not addressed.
Lib/plistlib.py
Outdated
result = [] | ||
self._objects[ref] = result | ||
result.extend(self._read_object(x) for x in obj_refs) | ||
return result |
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.
Why the return statement? This saves a duplicate store to self._objects[ref] at the end of the method, but makes handling this token different from he previous ones.
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.
The tokens "array" and "dict" are special. Since they are containers and can contain a cyclic reference to itself, self._objects[ref]
should be set before reading the content. These tokens already handled different from tokens for scalar types, and the difference will be kept even if remove the return statement here.
Lib/plistlib.py
Outdated
for k, o in zip(key_refs, obj_refs): | ||
result[self._read_object(self._object_offsets[k]) | ||
] = self._read_object(self._object_offsets[o]) | ||
result[self._read_object(k)] = self._read_object(o) | ||
return result |
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 guess the same applies to this (existing) return statement.
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
* Fixed saving bytearrays. * Identical objects will be saved only once. * Equal references will be load as identical objects. * Added support for saving and loading recursive data structures. (cherry picked from commit a897aee)
GH-4654 is a backport of this pull request to the 3.6 branch. |
Thank you for your review @ronaldoussoren. |
* Fixed saving bytearrays. * Identical objects will be saved only once. * Equal references will be load as identical objects. * Added support for saving and loading recursive data structures. (cherry picked from commit a897aee)
* Fixed saving bytearrays. * Identical objects will be saved only once. * Equal references will be load as identical objects. * Added support for saving and loading recursive data structures.. (cherry picked from commit a897aee)
* [3.4] bpo-32072: Fix issues with binary plists. (GH-4455) * Fixed saving bytearrays. * Identical objects will be saved only once. * Equal references will be load as identical objects. * Added support for saving and loading recursive data structures.. (cherry picked from commit a897aee) * Fix implementation dependent assertion in test_plistlib. (#4813) It is failed with an advanced optimizer.
* [3.5] bpo-32072: Fix issues with binary plists. (GH-4455) * Fixed saving bytearrays. * Identical objects will be saved only once. * Equal references will be load as identical objects. * Added support for saving and loading recursive data structures. (cherry picked from commit a897aee) * Fix implementation dependent assertion in test_plistlib. (#4813) It is failed with an advanced optimizer.
* Add missing test class (mistake in GH-4455) * Increase coverage with 4 more test cases * Rename neg_uid to huge_uid in test_modified_uid_huge * Replace test_main() with unittest.main() * Update plistlib docs
* Add missing test class (mistake in pythonGH-4455) * Increase coverage with 4 more test cases * Rename neg_uid to huge_uid in test_modified_uid_huge * Replace test_main() with unittest.main() * Update plistlib docs
* Add missing test class (mistake in pythonGH-4455) * Increase coverage with 4 more test cases * Rename neg_uid to huge_uid in test_modified_uid_huge * Replace test_main() with unittest.main() * Update plistlib docs
https://bugs.python.org/issue32072