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

src, test: fix V8 compiler warnings #24246

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 8, 2018

This pull request follows up on #24060 and attempts to fix all the remaining v8 warnings regarding Set/Get.

There is currently one warning still being generated from [src/node.cc](a525283#diff-cd53544f44aab2c697bcd7b6a57f23ccR675-677)

I've left a comment in the code as I was not sure how to get the correct context. Hopefully someone can help out and advise on this.

The commits are currently separate for each C++ source file but can be squashed unless anyone objects.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@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 related to general changes in the lib or src directory. labels Nov 8, 2018
@danbev
Copy link
Contributor Author

danbev commented Nov 8, 2018

src/node.cc Outdated Show resolved Hide resolved
@danbev
Copy link
Contributor Author

danbev commented Nov 8, 2018

Re-run of failing node-test-commit-windows-fanned.
Re-run of failing node-test-commit-freebsd.

@refack
Copy link
Contributor

refack commented Nov 8, 2018

can be squashed unless anyone objects.

+1 for squashing all Object::Set related commits, since they are logically a single action (refactor out use of a deprecated method);

@cjihrig
Copy link
Contributor

cjihrig commented Nov 10, 2018

@refack is this in a state that it can be rebased, squashed, and landed? The compiler warnings are currently a bit much 😄

@refack
Copy link
Contributor

refack commented Nov 10, 2018

@refack is this in a state that it can be rebased

Me or @danbev?

@refack
Copy link
Contributor

refack commented Nov 11, 2018

@danbev
Copy link
Contributor Author

danbev commented Nov 11, 2018

@cjihrig @refack I've rebased and squashed and will start a new CI hopefully get this merged after a successful CI run.

@danbev
Copy link
Contributor Author

danbev commented Nov 11, 2018

@danbev
Copy link
Contributor Author

danbev commented Nov 11, 2018

Re-run of failing /node-test-commit-freebsd ✔️

@danbev
Copy link
Contributor Author

danbev commented Nov 11, 2018

Landed in 344d33e, and faa584a.

@danbev danbev closed this Nov 11, 2018
@danbev danbev deleted the v8_compiler_warnings branch November 11, 2018 07:09
danbev added a commit that referenced this pull request Nov 11, 2018
This commit changes the code to use the maybe version.

PR-URL: #24246
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
danbev added a commit that referenced this pull request Nov 11, 2018
PR-URL: #24246
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
This commit changes the code to use the maybe version.

PR-URL: #24246
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24246
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
This commit changes the code to use the maybe version.

PR-URL: nodejs#24246
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@codebytere
Copy link
Member

@danbev this is a bit of a large backport and it definitely won't land cleanly, do you think it should be backported to v10.x?

@danbev
Copy link
Contributor Author

danbev commented Dec 17, 2018

@codebytere I think this can be skipped but if anyone thinks differently let me know and I can take a look at backporting it.

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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants