src: fix type mismatch warnings from missing priv#24737
src: fix type mismatch warnings from missing priv#24737sam-github wants to merge 2 commits intonodejs:masterfrom
Conversation
Registration initialization functions are expected to have a 4th argument, a void*, so add them where necessary to fix the warnings.
nodejs-github-bot
added
c++
addaleax
left a comment
addaleax
left a comment
There was a problem hiding this comment.
LGTM, but I’m curious, how/where does this generate warnings? I’ve never seen any…
|
I guess that's why nobody has fixed them, then. When I'm working in src/, I can't see errors or warnings in my code, they are lost behind these: |
|
It might just be a GCC 8 thing then. :) |
@addaleax What about the above, do you not see them, either? I see screen after screen of them. Its not clear to me how or even if they can be fixed. |
|
@sam-github I don’t, no. I think we should maybe turn off this type of warning, or at least use pragmas to silence them? I’m not sure we could fix that one, either. |
|
binary propogation failed on linux & arm: resumed the build |
|
Hmmm, let's try a full-on rebuild CI: https://ci.nodejs.org/job/node-test-pull-request/19126/ |
Trott
added
the
author ready
|
Given that this makes no changes that anything other than the compiler should notice, I don't think these are related:
I'm not sure, can this land? @Trott do we absolutely require green in CI? I'll rekick the CI for now. |
|
ci: https://ci.nodejs.org/job/node-test-pull-request/19157/ (rebuild of 19126) |
src/node_util.cc
Outdated
| } | ||
|
|
||
| static void GetPromiseDetails(const FunctionCallbackInfo<Value>& args) { | ||
| Environment* env = Environment::GetCurrent(args); |
There was a problem hiding this comment.
@addaleax Is it possible this call and the similar one below had side-effects? Its suspicious that https://ci.nodejs.org/job/node-test-commit-linux/nodes=alpine-last-latest-x64/23666/testReport/junit/(root)/test/parallel_test_util_callbackify/ is happening regularly, and depends on promise behaviours.
There was a problem hiding this comment.
@sam-github No, that seems very unlikely – the only side effect I could possibly think of is allocating a Local<> under the hood (which leaks into the calling HandleScope), but since these are JS bindings, there’s always a HandleScope here, and V8 doesn’t generally rely on them being used in a specific way
|
Rebuild Windows (only part that failed last time): https://ci.nodejs.org/job/node-test-commit-windows-fanned/22961/ |
^--- the Windows failures all look like that. |
|
tried to restart node-test-commit: https://ci.nodejs.org/job/node-test-commit/23921/ |
The Windows rebuild I did an hour ago came back yellow, so I think this can land. https://ci.nodejs.org/job/node-test-commit-windows-fanned/22961/ |
Registration initialization functions are expected to have a 4th argument, a void*, so add them where necessary to fix the warnings. PR-URL: #24737 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Registration initialization functions are expected to have a 4th argument, a void*, so add them where necessary to fix the warnings. PR-URL: #24737 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Registration initialization functions are expected to have a 4th argument, a void*, so add them where necessary to fix the warnings. PR-URL: nodejs#24737 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
6 participants
Registration initialization functions are expected to have a 4th
argument, a void*, so add them where necessary to fix the warnings.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes