Streamline object operations through a better SlotMap interface #1533
Conversation
Implement a "compute" method which allows a bunch of the logic in ScriptableObject work without as many slot map operations, especially when changing slot instance types. Also, get a particular check out of the slot map implementations that has always bothered me and move it to ScriptableObject for better encapsulation of language-specific behavior.
| Slot foundNewSlot = map.query("foo", 0); | ||
| assertEquals("Testing", foundNewSlot.value); | ||
| assertSame(foundNewSlot, newSlot); | ||
| map.remove("foo", 0); | ||
| map.compute("foo", 0, (k, ii, e) -> null); |
There was a problem hiding this comment.
Sorry, but for me this code looks absolutely unreadable - i guess nobody is able to understand what this line of code does. It might be technically clever to do it like this but from a maintainers point of view i need more support.
Please think (again) about better method naming and at least add some jdoc to this magic method.
There was a problem hiding this comment.
I never fully grasped how the Slots and SlotMaps worked and was wondering when I saw this PR coming by whether we could have some blogpost-style explainer of it, either in the Wiki or in the documentation
|
Hi @gbrail, Just my two cents... |
| @@ -363,14 +363,27 @@ public void delete(String name) { | |||
| @Override | |||
| public void delete(int index) { | |||
| checkNotSealed(null, index); | |||
| slotMap.remove(null, index); | |||
| slotMap.compute(null, index, ScriptableObject::checkSlotRemoval); | |||
There was a problem hiding this comment.
Can we already take into consideration here that eventually the delete operation needs to start returning true or false, indicating whether the delete indeed occurred or not?
I know the 'delete' method of the Scriptable interface has a void return type, but this is wrong from an EcmaScript pov, so it would be good to al least make sure that the internal implementation can already properly handle this
|
I'm really glad that we have a core of people reading and noticing what is
going on with Rhino!
Those are all fair comments. I should have left more time for others to
comment and review before doing anything. I'll leave more time in the
future and ask more for comments so that everyone has a chance to review.
(Also I have somehow had time in the evenings this week to work on Rhino,
and I can't always promise I won't have to go back to weekly updates at
some point!)
I'm open to putting an explicit "remove" back in the SlotMap interface if
it makes the code clearer. However, if the idea is to put it in because we
might need it later -- I'd actually rather hold off then. Would making this
bit more compatible require changes to the Scriptable class, or can we just
make vanilla (i.e. ScriptableObject-based) objects more compatible and
leave Scriptable as is for embedders?
Finally, I understand the comments about documentation and I will see when
I can find time to improve it. I wonder if you found the Javadoc in the
"SlotMap" class to be helpful?
…On Mon, Jul 22, 2024 at 11:59 PM Paul Bakker ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java
<#1533 (comment)>:
> Slot foundNewSlot = map.query("foo", 0);
assertEquals("Testing", foundNewSlot.value);
assertSame(foundNewSlot, newSlot);
- map.remove("foo", 0);
+ map.compute("foo", 0, (k, ii, e) -> null);
I never fully grasped how the Slots and SlotMaps worked and was wondering
when I saw this PR coming by whether we could have some blogpost-style
explainer of it, either in the Wiki or in the documentation
—
Reply to this email directly, view it on GitHub
<#1533 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I254MDD7VFQ4U4FKJZDZNX5NNAVCNFSM6AAAAABLHDFEEOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJTGEZTENJTGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
That was also my first idea - to have some readable wrappers but finally i think adding some jdoc to the method and maybe better naming (in this case maybe a really long name makes sense) will be fine for me |
|
My thinking at the time was to re-use the name and signature from the
java.util.Map interface -- in that case, there is literally a method called
"compute" that works exactly like the one in SlotMap. But I am a strange
person who reads Javadoc for fun and I realize that most Java programmers
probably don't know that!
With that said, understanding the requirements now, I could imagine a
simpler interface. Let me work on it and create a PR for people to look at,
along with at least better JavaDoc, if not eventually some better docs. I'm
away next week but this is kind of fun so stay tuned!
…On Tue, Jul 23, 2024 at 10:22 AM RBRi ***@***.***> wrote:
'm open to putting an explicit "remove" back in the SlotMap interface if
it makes the code clearer.
That was also my first idea - to have some readable wrappers but finally i
think adding some jdoc to the method and maybe better naming (in this case
maybe a really long name makes sense) will be fine for me
—
Reply to this email directly, view it on GitHub
<#1533 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I2Z7PWNJAKM64RXW55DZN2GN5AVCNFSM6AAAAABLHDFEEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBVHAYTSMZSGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Implement a "compute" method which allows a bunch of the logic in ScriptableObject work without as many slot map operations, especially when changing slot instance types.
Also, get a particular check out of the slot map implementations that has always bothered me and move it to ScriptableObject for better encapsulation of language-specific behavior.