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.

Sign up for GitHub

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

Jump to bottom

test: fix n-api addon build warnings #21808

Closed
wants to merge 1 commit into from
Closed

test: fix n-api addon build warnings #21808

wants to merge 1 commit into from

Conversation

kfarnung
Copy link
Contributor

@kfarnung kfarnung commented Jul 14, 2018 •
edited

Fixed an MSVC warning on Windows:

  • test_general.c - Lossy conversion from int64 to double, explicitly
    casting to double resolved the warning
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

@kfarnung kfarnung self-assigned this Jul 14, 2018
@nodejs-github-bot
Copy link
Collaborator

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

@kfarnung kfarnung requested a review from devsnek July 14, 2018 01:00
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Jul 14, 2018
@richardlau
Copy link
Member

Looks like the test_bigint.c changes are being taken care of in #21796.

@kfarnung
Copy link
Contributor Author

Duplicate of #21796

@kfarnung kfarnung marked this as a duplicate of #21796 Jul 16, 2018
@kfarnung kfarnung closed this Jul 16, 2018
@kfarnung kfarnung deleted the napiwarnings branch July 16, 2018 05:30
@kfarnung kfarnung restored the napiwarnings branch July 16, 2018 05:30
@kfarnung kfarnung deleted the napiwarnings branch July 16, 2018 05:30
@kfarnung kfarnung restored the napiwarnings branch July 16, 2018 05:30
@kfarnung kfarnung reopened this Jul 16, 2018
@kfarnung
Copy link
Contributor Author

I'll let #21796 land and rebase on those changes, the remaining one is purely to quiet a warning in MSVC.

@kfarnung
test: fix n-api addon build warnings
8d12e3a 
Fixed an MSVC warning on Windows:
* test_general.c - Lossy conversion from int64 to double, explicitly
  casting to double resolved the warning
@kfarnung kfarnung removed the request for review from devsnek July 16, 2018 21:10
mhdawson
mhdawson approved these changes Jul 17, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 18, 2018
@maclover7
Copy link
Contributor

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

@trivikr
Copy link
Member

trivikr commented Aug 3, 2018

Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/16173/

@maclover7
Copy link
Contributor

Landed in f8d34b9

@maclover7 maclover7 closed this Aug 4, 2018
maclover7 pushed a commit that referenced this pull request Aug 4, 2018
@kfarnung @maclover7
test: fix n-api addon build warnings
f8d34b9 
Fixed an MSVC warning on Windows:
* test_general.c - Lossy conversion from int64 to double, explicitly
  casting to double resolved the warning

PR-URL: #21808
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Aug 6, 2018
@kfarnung @targos
test: fix n-api addon build warnings
d38ccaa 
Fixed an MSVC warning on Windows:
* test_general.c - Lossy conversion from int64 to double, explicitly
  casting to double resolved the warning

PR-URL: #21808
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@rvagg rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees

@kfarnung kfarnung

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@kfarnung @nodejs-github-bot @richardlau @addaleax @maclover7 @trivikr @jasnell @cjihrig @mhdawson @Trott