-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
refactor: Change C style casts to C++ style (Part 5) #11688
Conversation
✅ Deploy Preview for meta-velox canceled.
|
b44c8ff
to
1bd6cd3
Compare
09807e0
to
f888c1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -1,9 +1,26 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedroerp : I was working with some of the files in the velox/tpch/gen/dbgen folder and there were some inconsistencies in the Facebook header placements. Some files (e.g. file https://github.com/facebookincubator/velox/blob/main/velox/tpch/gen/dbgen/include/dbgen/dss.h) had them, and some not. While working on this PR, as per instructions I did fix the headers I touched, but I wonder if the placement of the copyright is okay from a licensing perspective.
I am keen to hear your take on it. wdyt ?
velox/tpch/gen/dbgen/bm_utils.cpp
Outdated
@@ -398,7 +400,7 @@ char** mk_ascdate(void) { | |||
dss_time_t t; | |||
DSS_HUGE i; | |||
|
|||
m = (char**)malloc((size_t)(TOTDATE * sizeof(char*))); | |||
m = reinterpret_cast<char**>(malloc((size_t)(TOTDATE * sizeof(char*)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed one C-style cast in the malloc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Done.
9dde6f0
to
7b15200
Compare
9cd2060
to
9bdf1cb
Compare
f57c4ae
to
5f4a6ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @aditi-pandit
#define TPCH_QUERY(id) reinterpret_cast<const char*>(TPCH_QUERIES_##id) | ||
#define TPCH_ANSWER_SF1(id) reinterpret_cast<const char*>(TPCH_ANSWERS_SF1_##id) | ||
#define TPCH_ANSWER_SF0_1(id) reinterpret_cast<const char*>(TPCH_ANSWERS_SF0_1_##id) | ||
#define TPCH_ANSWER_SF0_01(id) reinterpret_cast<const char*>(TPCH_ANSWERS_SF0_01_##id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aditi-pandit i noticed you removed the undefs. Let's keep them to avoid leaking the pragmas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the defines and undefs closer to use, so that the scope is restricted to the minimum.
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@bikramSingh91 can you please re-import this? Thanks! |
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@aditi-pandit File: lines 1003 - 1012
line 2832
lines 2835 - 2845
line 13430
lines 13433 - 13443
lines 60731 - 60741
|
Thanks @bikramSingh91. Have made the changes. PTAL. |
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@aditi-pandit Unfortunately after you fixed the previous format changes, some more popped up. My guess is that format rules for macros is a bit wonky. Pasting them here again, hope it'll be the last of it. file: line 2827
line 2830 - 2835
line 13420
line 13423 - 13428
line 60716 - 60721
|
@bikramSingh91 : Did the next round of linter fixes :( Hope this resolves the issues. Thanks for your patience with this. |
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
One more popped up, this would be the last for sure! lines 60719 - 60720
|
@bikramSingh91 : Sounds like you don't need the linter fix from me. I was thinking that I will push a commit in any case, but don't want to cause further complications in the merge process. Definitely ping me if you need anything else. I've sent you a slack as well. |
As per the security guideline in https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast Covers the findings in velox/tpch
Thanks @bikramSingh91 for your message. Have updated the code accordingly. |
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@bikramSingh91 merged this pull request in 4c6ab14. |
As per the security guideline in
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast
Covers the findings in velox/tpch