Refactor PropertyDescriptor
#794
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #794 +/- ##
==========================================
- Coverage 59.68% 59.54% -0.15%
==========================================
Files 157 157
Lines 10054 10034 -20
==========================================
- Hits 6001 5975 -26
- Misses 4053 4059 +6
Continue to review full report at Codecov.
|
Benchmark for be7d439Click to view benchmark
|
Benchmark for f713b54Click to view benchmark
|
HalidOdat
force-pushed
the
refactor/property-descriptor
branch
5 times, most recently
from
d9455d4
to
634d624
Compare
Benchmark for 719e34fClick to view benchmark
|
Benchmark for 7ce1a65Click to view benchmark
|
Benchmark for b0b3f3fClick to view benchmark
|
|
Some benchmarks show a 20% increase in performance :) |
RageKnify
reviewed
Oct 7, 2020
RageKnify
reviewed
Oct 7, 2020
RageKnify
reviewed
Oct 7, 2020
|
You're leaving a lot of |
My intention with this PR was not to fix the already present bugs (even though I did fix some), but to make them more visible, places where there were hard to identify bugs (like accessor not being called, properties being ignored when they shouldn't) now panic (with unwrap/expect) so its easier to know were there located, and are immediately known as bugs, instead of giving some weird behaviour.
Will create the issue once this has been merged :) |
Benchmark for fa3799aClick to view benchmark
|
RageKnify
reviewed
Oct 8, 2020
Razican
approved these changes
Oct 8, 2020
There was a problem hiding this comment.
I love the new benchmarks. Check the comment by @RageKnify and see if a re-base fixes the tests, but for me it's good to go (also, I'm not super familiar with this, so take my review with a gran of salt) :)
RageKnify
approved these changes
Oct 8, 2020
- Add DataDescriptor - Add AccessorDescriptor - Remove PropertyDescriptor::data_descriptor() - Make AccessorDescriptor accessors GcObjects - Make Object::get_own_property return a Option<PropertyDescriptor> - Remove PropertyDescriptor::is_none() - Remove PropertyDescriptor::empty() - Removed PropertyDescriptor::value() - Added spec complaint ToPropertyDescriptor (to_property_descriptor) - Remove From<&PropertyDescriptor> for Value
HalidOdat
force-pushed
the
refactor/property-descriptor
branch
from
3ac48f3
to
760e0f4
Compare
Benchmark for 023fcdbClick to view benchmark
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds a nice API for creating Property Descriptor (
DataDescriptorandAccessorDescriptor) by making illegal state unrepresentable, for examplegetandvaluein the same property descriptor (which is not allowed, but was possible with the old implementation), which we needed dynamic checks to prove that it was not the case (performance penalty), but now we leverage the Type System for this as well as it makes implementing #591 and accessors so much easier.It changes the following:
Property=>PropertyDescriptorDataDescriptorfor defining data descriptorsAccessorDescriptorfor defining accessor descriptorsPropertyDescriptoran enumHAS_*formAttributeValue::update_propertyrelated to DecoupleObjectfromValue#577