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

src: replace c-style cast #26888

Merged
merged 1 commit into from
Apr 2, 2019
Merged

src: replace c-style cast #26888

merged 1 commit into from
Apr 2, 2019

Conversation

gengjiawen
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 24, 2019
addaleax
addaleax reviewed Mar 24, 2019
View reviewed changes
src/api/exceptions.cc Outdated Show resolved Hide resolved
src/api/exceptions.cc Outdated Show resolved Hide resolved
@gengjiawen gengjiawen force-pushed the replace_c_cast branch 2 times, most recently from 8f16c4b to fc3a773 Compare March 24, 2019 14:47
@BridgeAR 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
@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented 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

@targos 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
@gengjiawen
Copy link
Member Author

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.

@nodejs-github-bot
Copy link
Collaborator

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

refack
refack reviewed Mar 30, 2019
View reviewed changes
src/api/exceptions.cc Outdated
if (must_free)
LocalFree((HLOCAL)msg);
if (must_free) {
LocalFree(HLOCAL(msg));

This comment was marked as outdated.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Contributor

@refack refack Mar 31, 2019 •
edited
Loading

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

Suggested change
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 BridgeAR requested review from refack and addaleax April 1, 2019 20:32
@BridgeAR 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
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2019 •
edited by refack
Loading

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.

@refack
Copy link
Contributor

refack commented 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.

@refack 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
@gengjiawen
Copy link
Member Author

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 ?

@refack
Copy link
Contributor

refack commented Apr 2, 2019

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

refack
refack suggested changes Apr 2, 2019
View reviewed changes
src/api/exceptions.cc Outdated Show resolved Hide resolved
@refack refack self-assigned this Apr 2, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@gengjiawen @refack
src: replace c-style cast
4d27394 
PR-URL: nodejs#26888
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@refack refack merged commit 4d27394 into nodejs:master Apr 2, 2019
@refack refack removed their assignment Apr 2, 2019
@gengjiawen gengjiawen deleted the replace_c_cast branch April 2, 2019 23:24
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
@gengjiawen @BethGriggs
src: replace c-style cast
24fb0d8 
PR-URL: #26888
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
@gengjiawen @BethGriggs
src: replace c-style cast
0fa9e03 
PR-URL: #26888
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
@gengjiawen @BethGriggs
src: replace c-style cast
aa6a741 
PR-URL: #26888
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 10, 2019
@gengjiawen @BethGriggs
src: replace c-style cast
054cf7b 
PR-URL: #26888
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gengjiawen @nodejs-github-bot @targos @refack @jasnell @addaleax @BridgeAR