src: fix broken fast callback signatures#59055
src: fix broken fast callback signatures#59055Renegade334 wants to merge 4 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Renegade334
force-pushed
the
fix-histogram-fast-callbacks
branch
3 times, most recently
from
93048b8 to
9926389
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59055 +/- ##
==========================================
+ Coverage 90.04% 90.10% +0.06%
==========================================
Files 648 648
Lines 191078 191075 -3
Branches 37452 37457 +5
==========================================
+ Hits 172050 172164 +114
+ Misses 11649 11534 -115
+ Partials 7379 7377 -2
🚀 New features to boost your workflow:
|
|
It seems to me that #54408 should be reverted as a whole, rather than reverting partially, e.g. the documentations. |
|
The suggestion about passing receiver as the second argument at https://github.com/nodejs/node/blob/main/doc/contributing/adding-v8-fast-api.md#requirements is still there, should it be reverted as well? Otherwise the change in this PR is not aligning with the suggestion. |
|
It's removed in #58999, or are you referring to this PR? |
Renegade334
force-pushed
the
fix-histogram-fast-callbacks
branch
from
9926389 to
be6287b
Compare
| if (value < 1) { | ||
| HandleScope scope(options.isolate); | ||
| THROW_ERR_OUT_OF_RANGE(options.isolate, "value is out of range"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
value is validated in the JS layer, so an assertion check should be fine here.
|
cc @nodejs/cpp-reviewers – looking for reviews if possible. |
3 participants
Since #54408, the various fast callbacks that were edited by that PR have broken signatures and have not been invoked. (The changed signatures would only have been valid if the receiver were prepended to the function arguments wherever those bindings were called from JS, which isn't the case.)
There was no debug tracking of these callbacks, so this went unnoticed at the time.
This change fixes the broken signatures and adds debug testing to validate the fast paths.
This effectively reverts the remaining source changes from #54408 that were not already covered by #58054.