GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: fix type mismatch warnings from missing priv by sam-github · Pull Request #24737 · nodejs/node

Conversation

@sam-github

@sam-github sam-github commented Nov 30, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@sam-github
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

nodejs-github-bot commented Nov 30, 2018

Copy link
Copy Markdown
Collaborator

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot 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
addaleax approved these changes Nov 30, 2018
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I’m curious, how/where does this generate warnings? I’ve never seen any…

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sam-github

sam-github commented Nov 30, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented Nov 30, 2018

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig
cjihrig approved these changes Nov 30, 2018
View reviewed changes
@sam-github

sam-github commented Nov 30, 2018

Copy link
Copy Markdown
Contributor Author
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented Nov 30, 2018

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Nov 30, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Nov 30, 2018

Copy link
Copy Markdown
Contributor Author

binary propogation failed on linux & arm: resumed the build

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Dec 2, 2018

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

sam-github commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/19157/ (rebuild of 19126)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sam-github
sam-github commented Dec 3, 2018
View reviewed changes
Comment thread src/node_util.cc Outdated
}

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

@sam-github sam-github Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Dec 3, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sam-github
fixup! src: fix type mismatch warnings from missing priv
6e15034
@sam-github

sam-github commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Dec 4, 2018

Copy link
Copy Markdown
Member

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

sam-github reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Dec 4, 2018

Copy link
Copy Markdown
Contributor Author
15:49:04 c:\workspace\node-test-binary-windows>git fetch --no-tags [email protected]: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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Dec 4, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Dec 4, 2018

Copy link
Copy Markdown
Member

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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Dec 4, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 0c65314, thanks @Trott

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github sam-github closed this Dec 4, 2018
@sam-github
sam-github deleted the src-warning-cleanup branch December 4, 2018 03:44
sam-github added a commit that referenced this pull request Dec 4, 2018
@sam-github
src: fix type mismatch warnings from missing priv
0c65314
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 pushed a commit that referenced this pull request Dec 5, 2018
@sam-github @BridgeAR
src: fix type mismatch warnings from missing priv
5bd33f1
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
v11.4.0 proposal #24854
Merged
4 tasks done
This was referenced Dec 7, 2018
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#261
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#262
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#263
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@sam-github @refack
src: fix type mismatch warnings from missing priv
f16e1cf
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@BethGriggs BethGriggs added the backport-requested-v10.x label Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@sam-github @nodejs-github-bot @addaleax @Trott @cjihrig @BethGriggs
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.