src: fix type mismatch warnings from missing priv by sam-github · Pull Request #24737 · nodejs/node · GitHub

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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Registration initialization functions are expected to have a 4th argument, a void*, so add them where necessary to fix the warnings.

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1734/pipeline

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:

  g++ -o /home/sam/w/core/node/out/Release/obj.target/node_lib/src/tty_wrap.o ../src/tty_wrap.cc '-DV8_DEPRECATION_WARNINGS' '-DV8_IMMINENT_DEPRECATION_WARNINGS' '-DOPENSSL_T
HREADS' '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' '-DHAVE_INSPECTOR=1' '-D__POSIX__' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_HAVE_SMALL_ICU=1' '-DHAVE_OPENSSL=1' '-DUCONFIG_NO_SERVICE=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=1' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DHTTP_PARSER_STRICT=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' '-DNGHTTP2_STATICLIB' -I../src -I/home/sam/w/core/node/out/Release/obj/gen -I/home/sam/w/core/node/out/Release/obj/gen/include -I/home/sam/w/core/node/out/Release/obj/gen/src -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/http_parser -I../deps/cares/include -I../deps/uv/include -I../deps/nghttp2/lib/includes -I../deps/openssl/openssl/include  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -Wall -Wextra -Wno-unused-parameter -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++1y -MMD -MF /home/sam/w/core/node/out/Release/.deps//home/sam/w/core/node/out/Release/obj.target/node_lib/src/tty_wrap.o.d.raw   -c
           In file included from ../src/async_wrap-inl.h:29,                                                                                                                             
                 from ../src/stream_base.h:7,                                                                                                                                                  from ../src/stream_wrap.h:27,                                                                                                                                                 
from ../src/tty_wrap.h:29,                                                                                                                                                    
from ../src/tty_wrap.cc:22:                                                                                                                                  
../src/node_internals.h:159:49: warning: cast between incompatible function types from ‘void (*)
(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Context>)’ to 
‘node::addon_context_register_func’ {aka ‘void (*)(v8::Local<v8::Object>, v8::Local<v8::Value>, 
v8::Local<v8::Context>, void*)’} [-Wcast-function-type]                                
(node::addon_context_register_func) (regfunc),                            \                                                                                                                                               ^                                                                                                                            
../src/node_internals.h:786:3: note: in expansion of macro ‘NODE_MODULE_CONTEXT_AWARE_CPP’                                                                                       
NODE_MODULE_CONTEXT_AWARE_CPP(modname, regfunc, nullptr, NM_F_INTERNAL)                                                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                                                                                                              
../src/tty_wrap.cc:173:1: note: in expansion of macro 
‘NODE_MODULE_CONTEXT_AWARE_INTERNAL’                                                                                     
NODE_MODULE_CONTEXT_AWARE_INTERNAL(tty_wrap, node::TTYWrap::Initialize)

core/node (master u=) % g++ --version; uname -a
g++ (Ubuntu 8.2.0-7ubuntu1) 8.2.0
Linux samtu 4.18.0-11-generic #12-Ubuntu SMP Tue Oct 23 19:22:37 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

It might just be a GCC 8 thing then. :)

In file included from ../src/node.h:63,
                 from ../src/node_crypto.h:27,
                 from ../src/node_crypto_bio.h:27,
                 from ../src/node_crypto_bio.cc:22:
../deps/v8/include/v8.h: In instantiation of ‘void v8::PersistentBase<T>::SetWeak(P*, typename v8::WeakCallbackInfo<P>::Callback, v8::WeakCallbackType) [with P = node::BaseObject; T = v8::Object; typename v8::WeakCallbackInfo<P>::Callback = void (*)(const v8::WeakCallbackInfo<node::BaseObject>&)]’:
../src/base_object-inl.h:104:42:   required from here
../deps/v8/include/v8.h:9707:16: warning: cast between incompatible function types from ‘v8::WeakCallbackInfo<node::BaseObject>::Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<node::BaseObject>&)’} to ‘Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<void>&)’} [-Wcast-function-type]
                reinterpret_cast<Callback>(callback), type);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@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.

ci: https://ci.nodejs.org/job/node-test-pull-request/19092/

binary propogation failed on linux & arm: resumed the build

ci: https://ci.nodejs.org/job/node-test-pull-request/19096/

Hmmm, let's try a full-on rebuild CI: https://ci.nodejs.org/job/node-test-pull-request/19126/

Given that this makes no changes that anything other than the compiler should notice, I don't think these are related:

https://ci.nodejs.org/job/node-test-commit-linux/nodes=alpine-last-latest-x64/23666/testReport/junit/(root)/test/parallel_test_util_callbackify/ flaky test?
https://ci.nodejs.org/job/node-test-binary-arm/4590/ pi1-docker failed across the board?

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)

ci: https://ci.nodejs.org/job/node-test-pull-request/19161/ (new)

@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.

@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

resume ci: https://ci.nodejs.org/job/node-test-commit/23915/

Rebuild Windows (only part that failed last time): https://ci.nodejs.org/job/node-test-commit-windows-fanned/22961/

15:49:04 c:\workspace\node-test-binary-windows>git fetch --no-tags binary_tmp@147.75.70.237:binary_tmp.git +refs/heads/jenkins-node-test-commit-windows-fanned-6e1503496847634a8b9ff0ed6f431f0d5c728a88-bin-win-vs2017:refs/remotes/jenkins_tmp 
15:49:05 error: refs/heads/jenkins-node-test-commit-arm-fanned-5db70cfafe955fd948736d77214b63cf10466286-binary-pi1p/cc-armv6 does not point to a valid object!
15:49:05 fatal: Couldn't find remote ref refs/heads/jenkins-node-test-commit-windows-fanned-6e1503496847634a8b9ff0ed6f431f0d5c728a88-bin-win-vs2017

^--- the Windows failures all look like that.

tried to restart node-test-commit: https://ci.nodejs.org/job/node-test-commit/23921/

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/

Landed in 0c65314, thanks @Trott

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>

src: fix type mismatch warnings from missing priv

5db70cf

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++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Nov 30, 2018

addaleax approved these changes Nov 30, 2018

View reviewed changes

cjihrig approved these changes Nov 30, 2018

View reviewed changes

Trott added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 2, 2018

sam-github commented Dec 3, 2018

View reviewed changes

fixup! src: fix type mismatch warnings from missing priv

6e15034

sam-github closed this Dec 4, 2018

sam-github deleted the src-warning-cleanup branch December 4, 2018 03:44

This was referenced Dec 5, 2018

CI failures: 20181201-20181205 nodejs/reliability#19

Closed

addon build failures on Windows machines nodejs/build#1622

Closed

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

BethGriggs added the backport-requested-v10.x label Mar 14, 2019

               }
               static void GetPromiseDetails(const FunctionCallbackInfo<Value>& args) {
-                Environment* env = Environment::GetCurrent(args);

src: fix type mismatch warnings from missing priv by sam-github · Pull Request #24737 · nodejs/node · GitHub

Uh oh!

Uh oh!

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:masternodejs/node:masterfrom sam-github:src-warning-cleanupsam-github/node:src-warning-cleanupCopy head branch name to clipboard

Conversation

sam-github commented Nov 30, 2018

Checklist

Uh oh!

nodejs-github-bot commented Nov 30, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Nov 30, 2018

Uh oh!

sam-github commented Nov 30, 2018

Uh oh!

addaleax commented Nov 30, 2018

Uh oh!

sam-github commented Nov 30, 2018

Uh oh!

sam-github commented Nov 30, 2018

Uh oh!

Trott commented Dec 2, 2018

Uh oh!

sam-github commented Dec 3, 2018

Uh oh!

sam-github commented Dec 3, 2018

Uh oh!

sam-github commented Dec 3, 2018

Uh oh!

sam-github Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Dec 3, 2018

Uh oh!

Trott commented Dec 4, 2018

Uh oh!

sam-github commented Dec 4, 2018

Uh oh!

sam-github commented Dec 4, 2018

Uh oh!

Trott commented Dec 4, 2018

Uh oh!

sam-github commented Dec 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src: fix type mismatch warnings from missing priv#24737
sam-github wants to merge 2 commits into
nodejs:masterfrom
sam-github:src-warning-cleanup

sam-github commented Nov 30, 2018 •
edited
Loading

addaleax Dec 3, 2018 •
edited
Loading