Skip to content

Fix/nullptr error message#228

Closed
ebickle wants to merge 2 commits intonodejs:masterfrom
ebickle:fix/nullptr-error-message
Closed

Fix/nullptr error message#228
ebickle wants to merge 2 commits intonodejs:masterfrom
ebickle:fix/nullptr-error-message

Conversation

@ebickle
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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