Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

kasicka
Copy link

@kasicka kasicka commented Feb 5, 2020

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

danbev and 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-bot nodejs-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
Member

addaleax commented Feb 5, 2020

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.

@addaleax addaleax closed this Feb 5, 2020
@kasicka
Copy link
Author

kasicka commented Feb 5, 2020

Was a PR needed for v12?

@addaleax
Copy link
Member

addaleax commented Feb 5, 2020

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

kasicka commented Feb 5, 2020

Okay, thanks for clarifying.

Sign up for free to 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