Skip to content

[v13.x backport] src: ignore GCC -Wcast-function-type for v8.h - #31649

Closed
kasicka wants to merge 2 commits into
nodejs:v13.x-stagingfrom
kasicka:backport-31475-31524-13.x
Closed

[v13.x backport] src: ignore GCC -Wcast-function-type for v8.h#31649
kasicka wants to merge 2 commits into
nodejs:v13.x-stagingfrom
kasicka:backport-31475-31524-13.x

Conversation

@kasicka

Copy link
Copy Markdown
Checklist

Backport of #31475 and #31524
Builds fine with GCC 9.2.1 and 7.3.1

  • 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

danbevand others added 2 commits February 5, 2020 16:13
This commit suggests that cast-function-type warnings be ignored
from v8.h. Currently, GCC reports a number of warnings like this:
In file included from ../src/util.h:27,
from ../src/aliased_buffer.h:7,
from ../src/memory_tracker.h:5,
from ../src/base_object.h:27,
from ../src/async_wrap.h:27,
from ../src/req_wrap.h:6,
from ../src/req_wrap-inl.h:6,
from ../src/connect_wrap.h:6,
from ../src/connect_wrap.cc:1:
../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:123:42: required from here
../deps/v8/include/v8.h:10374: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);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The motivation for doing this that it makes it difficult to spot
other warnings that might be important. Since it is v8 that
performs this cast I was not able to find a way around it.
PR-URL: #31475
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: #31517
PR-URL: #31524
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v13.x labels Feb 5, 2020
@addaleax

Copy link
Copy Markdown
Member

Thanks! These seem to land without conflicts on v13.x-staging, so I don’t think a separate backport PR is necessary here.

I’ve pushed the commits from master to v13.x-staging.

@addaleaxaddaleax closed this Feb 5, 2020
@kasicka

Copy link
Copy Markdown
Author

Was a PR needed for v12?

@addaleax

Copy link
Copy Markdown
Member

@kasicka If you didn’t run into any merge conflicts, probably not. Usually, PRs get picked up into LTS releases like v12.x after they have been in a Current release for two weeks. It can be nice to comment in the original PR(s) in the cases in which you do need a backport to v12.x, though, just to let people know that it’s important to you and sometimes to skip that waiting period – opening explicit backport PRs basically does that, too.

@kasicka

Copy link
Copy Markdown
Author

Okay, thanks for clarifying.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@kasicka@addaleax@nodejs-github-bot@danbev@lundibundi