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

Fix/nullptr error message #228

Closed
wants to merge 2 commits into from

Conversation

ebickle
Copy link

@ebickle ebickle commented Feb 11, 2018

Fixes #224.

The Napi::Error constructor was initializing the std::string _message private member variable to nullptr. As a class, std::string automatically initializes to the equivalent of a blank string. _message isn't a pointer; assigning a pointer literal (nullptr) to it causes static analysis warnings.

I've also enabled Visual C++ Static Analysis (Prefast) in the test suite to detect these sorts of problems. If that isn't desirable let me know and I'll back out that part of the PR.

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 assuming we don't have any other warnings/from existing warnings/errors from the static analysis.

Copy link
Contributor

@digitalinfinity digitalinfinity left a comment

Choose a reason for hiding this comment

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

LGTM although I echo @mhdawson's comments- @ebickle is prefast clean with this change?

@ebickle
Copy link
Author

ebickle commented Feb 14, 2018

@mhdawson Prefast is clean but there are regular compiler warnings relating to lossy implicit conversions to double. Only affects the unit test code, not the main source code itself.

mhdawson pushed a commit that referenced this pull request Mar 1, 2018
Enabled VC++ static analysis

PR-URL: #228
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@mhdawson
Copy link
Member

mhdawson commented Mar 1, 2018

Landed as faf19c4

@mhdawson mhdawson closed this Mar 1, 2018
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Enabled VC++ static analysis

PR-URL: nodejs/node-addon-api#228
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
Enabled VC++ static analysis

PR-URL: nodejs/node-addon-api#228
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
Enabled VC++ static analysis

PR-URL: nodejs/node-addon-api#228
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Enabled VC++ static analysis

PR-URL: nodejs/node-addon-api#228
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants