src: replace c-style cast by gengjiawen · Pull Request #26888 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Checklist

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

nodejs-github-bot added the c++

Issues and PRs that require attention from people who are familiar with C++.

label

Mar 24, 2019

BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 25, 2019

This is failing on Windows:

17:28:56 c:\workspace\node-compile-windows\src\api\exceptions.cc(240): error C2440: 'reinterpret_cast': cannot convert from 'const char *' to 'HLOCAL' [c:\workspace\node-compile-windows\node_lib.vcxproj]
17:28:56   c:\workspace\node-compile-windows\src\api\exceptions.cc(240): note: Conversion loses qualifiers

targos removed the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 30, 2019

This is failing on Windows:

17:28:56 c:\workspace\node-compile-windows\src\api\exceptions.cc(240): error C2440: 'reinterpret_cast': cannot convert from 'const char *' to 'HLOCAL' [c:\workspace\node-compile-windows\node_lib.vcxproj]
17:28:56   c:\workspace\node-compile-windows\src\api\exceptions.cc(240): note: Conversion loses qualifiers

I make the change to originally suggested by Visual Studio, Can you trigger thew windows build ? thanks.

if (must_free)
LocalFree((HLOCAL)msg);
if (must_free) {
LocalFree(HLOCAL(msg));

This comment was marked as outdated.

Choose a reason for hiding this comment

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

Done :)

Choose a reason for hiding this comment

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

I think we should leave this with

LocalFree(HLOCAL(msg));
LocalFree((HLOCAL)msg); // NOLINT(google-readability-casting)

because there is a bug in the API and what we really need here is:

LocalFree(const_cast(char*>(msg));

but that's just as bad for tidy.

BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Apr 1, 2019

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

[refack]
Windows fail is real:

 c:\workspace\node-compile-windows\src\api\exceptions.cc(240): error C2440: 'reinterpret_cast': cannot convert from 'const char *' to 'HLOCAL' [c:\workspace\node-compile-windows\node_lib.vcxproj]

Because what we really need is a const_cast.

FTR: This probably need using // NOLINT(google-readability-casting), but cpplint.py rejects NOLINT types it does not recognize. I'm planning on fixing that.

refack removed the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Apr 1, 2019

FTR: This probably need using // NOLINT(google-readability-casting), but cpplint.py rejects NOLINT types it does not recognize. I'm planning on fixing that.

Revert to the original one and add // NOLINT(google-readability-casting) after we resolve cpplint issue ?

Revert to the original one and add // NOLINT(google-readability-casting) after we resolve cpplint issue ?

Thinking about this again, let's just use the const_cast and at some point in the future fix all const_casts

PR-URL: nodejs#26888 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Refael Ackermann refack@gmail.com Reviewed-By: Anna Henningsen anna@addaleax.net

BethGriggs pushed a commit that referenced this pull request

Apr 5, 2019

PR-URL: #26888 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Refael Ackermann refack@gmail.com Reviewed-By: Anna Henningsen anna@addaleax.net

BethGriggs pushed a commit that referenced this pull request

Apr 9, 2019

BethGriggs pushed a commit that referenced this pull request

Apr 9, 2019

BethGriggs pushed a commit that referenced this pull request

Apr 10, 2019

Labels

c++

Issues and PRs that require attention from people who are familiar with C++.