lib: make new SafeSet(arr) safe#57895
Conversation
ljharb
added
the
lib / src
|
Review requested:
|
ljharb
force-pushed
the
safeset
branch
2 times, most recently
from
d57bd99 to
fa04710
Compare
|
(i assume the coverage failures mean i need to add some tests; if nobody has any objections to the PR then i'll ofc add those before attempting to land) |
|
It would be worth specifying in node/doc/contributing/primordials.md Lines 429 to 430 in c42d846 and probably add it to the list of culprit for performance issues (I guess we should first established whether it's indeed the case) node/doc/contributing/primordials.md Lines 108 to 113 in c42d846 Also, build is failing. |
|
Regarding the primordials.md comment, those are about the builtins, not the SafeX constructors, so I don't think it makes sense there - and I agree that we should wait until performance issues are identified before noting a performance culprit. I believe I've addressed the rest of the concerns (except probably the failing build, which I'll iterate on). I haven't applied this to SafeWeakSet, because there's only 2 instances of it in the codebase and neither are adding at construction time - I'm happy to add it in the future if it proves useful. |
This section is using the |
|
ah, true. should i change the example to something else, and then add a note indicating that SafeSet is immune to this? |
JakobJingleheimer
left a comment
JakobJingleheimer
left a comment
There was a problem hiding this comment.
oow, nice ergonomic improvement indeed 🙌
| constructor(i) { super(i); } // eslint-disable-line no-useless-constructor | ||
| constructor(i) { | ||
| super( | ||
| i && ArrayIsArray(i) ? |
There was a problem hiding this comment.
nit: the truthy test is probably superfluous. I can't find the implementation of ArrayIsArray, but it probably already does that, so this micro-op is likely actually a micro-deop.
There was a problem hiding this comment.
my assumption is that a truthiness check is much cheaper than any function call, but if a benchmark indicates that's untrue i'd be happy to change it in the future.
|
This pull request has been marked as stale due to 210 days of inactivity. |
10 participants
This has the direct ergonomic benefit of being able to safely pass an array to
new SafeSet().It has the unintended but desirable benefit of making a lot of existing post-startup calls to
new SafeSetwith an array more robust.This helps with #57876, and whichever lands first, the other will want to rebase on top of it.