refactor: Change C style casts to C++ style (Part 5) #11688

aditi-pandit · 2024-11-28T07:02:54Z

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

netlify · 2024-11-28T07:03:11Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`67c7ddb`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/675a55bf51afd00008e0ff90

czentgr

Thanks!

aditi-pandit · 2024-12-02T03:57:47Z

velox/tpch/gen/dbgen/include/tpch_constants.hpp

@@ -1,9 +1,26 @@
+/*


@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 ?

czentgr · 2024-12-02T20:44:00Z

velox/tpch/gen/dbgen/bm_utils.cpp

@@ -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*))));


Missed one C-style cast in the malloc.

Nice catch. Done.

velox/tpch/gen/dbgen/include/tpch_constants.hpp

velox/tpch/gen/dbgen/speed_seed.cpp

majetideepak

Thanks, @aditi-pandit

majetideepak · 2024-12-10T23:42:55Z

velox/tpch/gen/dbgen/include/tpch_constants.hpp

+#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)


@aditi-pandit i noticed you removed the undefs. Let's keep them to avoid leaking the pragmas.

Moved the defines and undefs closer to use, so that the scope is restricted to the minimum.

facebook-github-bot · 2024-12-10T23:43:03Z

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

majetideepak · 2024-12-11T10:36:57Z

@bikramSingh91 can you please re-import this? Thanks!

facebook-github-bot · 2024-12-11T21:24:33Z

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

bikramSingh91 · 2024-12-12T00:53:25Z

@aditi-pandit
There are a couple of format issues highlighted by internal linter, posting them here, can you please make the suggested changes? Thanks!

File: velox/tpch/gen/dbgen/include/tpch_constants.hpp

lines 1003 - 1012


-    TPCH_QUERY(q01), TPCH_QUERY(q02),
-    TPCH_QUERY(q03), TPCH_QUERY(q04),
-    TPCH_QUERY(q05), TPCH_QUERY(q06),
-    TPCH_QUERY(q07), TPCH_QUERY(q08),
-    TPCH_QUERY(q09), TPCH_QUERY(q10),
-    TPCH_QUERY(q11), TPCH_QUERY(q12),
-    TPCH_QUERY(q13), TPCH_QUERY(q14),
-    TPCH_QUERY(q15), TPCH_QUERY(q16),
-    TPCH_QUERY(q17), TPCH_QUERY(q18),
-    TPCH_QUERY(q19), TPCH_QUERY(q20),
+    TPCH_QUERY(q01), TPCH_QUERY(q02), TPCH_QUERY(q03), TPCH_QUERY(q04),
+    TPCH_QUERY(q05), TPCH_QUERY(q06), TPCH_QUERY(q07), TPCH_QUERY(q08),
+    TPCH_QUERY(q09), TPCH_QUERY(q10), TPCH_QUERY(q11), TPCH_QUERY(q12),
+    TPCH_QUERY(q13), TPCH_QUERY(q14), TPCH_QUERY(q15), TPCH_QUERY(q16),
+    TPCH_QUERY(q17), TPCH_QUERY(q18), TPCH_QUERY(q19), TPCH_QUERY(q20),

line 2832

-#define TPCH_ANSWER_SF0_01(id) reinterpret_cast<const char*>(TPCH_ANSWERS_SF0_01_##id)
+#define TPCH_ANSWER_SF0_01(id) \
+  reinterpret_cast<const char*>(TPCH_ANSWERS_SF0_01_##id)

lines 2835 - 2845

-    TPCH_ANSWER_SF0_01(q01), TPCH_ANSWER_SF0_01(q02),
-    TPCH_ANSWER_SF0_01(q03), TPCH_ANSWER_SF0_01(q04),
-    TPCH_ANSWER_SF0_01(q05), TPCH_ANSWER_SF0_01(q06),
-    TPCH_ANSWER_SF0_01(q07), TPCH_ANSWER_SF0_01(q08),
-    TPCH_ANSWER_SF0_01(q09), TPCH_ANSWER_SF0_01(q10),
-    TPCH_ANSWER_SF0_01(q11), TPCH_ANSWER_SF0_01(q12),
-    TPCH_ANSWER_SF0_01(q13), TPCH_ANSWER_SF0_01(q14),
-    TPCH_ANSWER_SF0_01(q15), TPCH_ANSWER_SF0_01(q16),
-    TPCH_ANSWER_SF0_01(q17), TPCH_ANSWER_SF0_01(q18),
-    TPCH_ANSWER_SF0_01(q19), TPCH_ANSWER_SF0_01(q20),
-    TPCH_ANSWER_SF0_01(q21), TPCH_ANSWER_SF0_01(q22)};
+    TPCH_ANSWER_SF0_01(q01), TPCH_ANSWER_SF0_01(q02), TPCH_ANSWER_SF0_01(q03),
+    TPCH_ANSWER_SF0_01(q04), TPCH_ANSWER_SF0_01(q05), TPCH_ANSWER_SF0_01(q06),
+    TPCH_ANSWER_SF0_01(q07), TPCH_ANSWER_SF0_01(q08), TPCH_ANSWER_SF0_01(q09),
+    TPCH_ANSWER_SF0_01(q10), TPCH_ANSWER_SF0_01(q11), TPCH_ANSWER_SF0_01(q12),
+    TPCH_ANSWER_SF0_01(q13), TPCH_ANSWER_SF0_01(q14), TPCH_ANSWER_SF0_01(q15),
+    TPCH_ANSWER_SF0_01(q16), TPCH_ANSWER_SF0_01(q17), TPCH_ANSWER_SF0_01(q18),
+    TPCH_ANSWER_SF0_01(q19), TPCH_ANSWER_SF0_01(q20), TPCH_ANSWER_SF0_01(q21),
+    TPCH_ANSWER_SF0_01(q22)};

line 13430

-#define TPCH_ANSWER_SF0_1(id) reinterpret_cast<const char*>(TPCH_ANSWERS_SF0_1_##id)
+#define TPCH_ANSWER_SF0_1(id) \
+  reinterpret_cast<const char*>(TPCH_ANSWERS_SF0_1_##id)

lines 13433 - 13443

-    TPCH_ANSWER_SF0_1(q01), TPCH_ANSWER_SF0_1(q02),
-    TPCH_ANSWER_SF0_1(q03), TPCH_ANSWER_SF0_1(q04),
-    TPCH_ANSWER_SF0_1(q05), TPCH_ANSWER_SF0_1(q06),
-    TPCH_ANSWER_SF0_1(q07), TPCH_ANSWER_SF0_1(q08),
-    TPCH_ANSWER_SF0_1(q09), TPCH_ANSWER_SF0_1(q10),
-    TPCH_ANSWER_SF0_1(q11), TPCH_ANSWER_SF0_1(q12),
-    TPCH_ANSWER_SF0_1(q13), TPCH_ANSWER_SF0_1(q14),
-    TPCH_ANSWER_SF0_1(q15), TPCH_ANSWER_SF0_1(q16),
-    TPCH_ANSWER_SF0_1(q17), TPCH_ANSWER_SF0_1(q18),
-    TPCH_ANSWER_SF0_1(q19), TPCH_ANSWER_SF0_1(q20),
-    TPCH_ANSWER_SF0_1(q21), TPCH_ANSWER_SF0_1(q22)};
+    TPCH_ANSWER_SF0_1(q01), TPCH_ANSWER_SF0_1(q02), TPCH_ANSWER_SF0_1(q03),
+    TPCH_ANSWER_SF0_1(q04), TPCH_ANSWER_SF0_1(q05), TPCH_ANSWER_SF0_1(q06),
+    TPCH_ANSWER_SF0_1(q07), TPCH_ANSWER_SF0_1(q08), TPCH_ANSWER_SF0_1(q09),
+    TPCH_ANSWER_SF0_1(q10), TPCH_ANSWER_SF0_1(q11), TPCH_ANSWER_SF0_1(q12),
+    TPCH_ANSWER_SF0_1(q13), TPCH_ANSWER_SF0_1(q14), TPCH_ANSWER_SF0_1(q15),
+    TPCH_ANSWER_SF0_1(q16), TPCH_ANSWER_SF0_1(q17), TPCH_ANSWER_SF0_1(q18),
+    TPCH_ANSWER_SF0_1(q19), TPCH_ANSWER_SF0_1(q20), TPCH_ANSWER_SF0_1(q21),
+    TPCH_ANSWER_SF0_1(q22)};

lines 60731 - 60741

-    TPCH_ANSWER_SF1(q01), TPCH_ANSWER_SF1(q02),
-    TPCH_ANSWER_SF1(q03), TPCH_ANSWER_SF1(q04),
-    TPCH_ANSWER_SF1(q05), TPCH_ANSWER_SF1(q06),
-    TPCH_ANSWER_SF1(q07), TPCH_ANSWER_SF1(q08),
-    TPCH_ANSWER_SF1(q09), TPCH_ANSWER_SF1(q10),
-    TPCH_ANSWER_SF1(q11), TPCH_ANSWER_SF1(q12),
-    TPCH_ANSWER_SF1(q13), TPCH_ANSWER_SF1(q14),
-    TPCH_ANSWER_SF1(q15), TPCH_ANSWER_SF1(q16),
-    TPCH_ANSWER_SF1(q17), TPCH_ANSWER_SF1(q18),
-    TPCH_ANSWER_SF1(q19), TPCH_ANSWER_SF1(q20),
-    TPCH_ANSWER_SF1(q21), TPCH_ANSWER_SF1(q22)};
+    TPCH_ANSWER_SF1(q01), TPCH_ANSWER_SF1(q02), TPCH_ANSWER_SF1(q03),
+    TPCH_ANSWER_SF1(q04), TPCH_ANSWER_SF1(q05), TPCH_ANSWER_SF1(q06),
+    TPCH_ANSWER_SF1(q07), TPCH_ANSWER_SF1(q08), TPCH_ANSWER_SF1(q09),
+    TPCH_ANSWER_SF1(q10), TPCH_ANSWER_SF1(q11), TPCH_ANSWER_SF1(q12),
+    TPCH_ANSWER_SF1(q13), TPCH_ANSWER_SF1(q14), TPCH_ANSWER_SF1(q15),
+    TPCH_ANSWER_SF1(q16), TPCH_ANSWER_SF1(q17), TPCH_ANSWER_SF1(q18),
+    TPCH_ANSWER_SF1(q19), TPCH_ANSWER_SF1(q20), TPCH_ANSWER_SF1(q21),
+    TPCH_ANSWER_SF1(q22)};

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

aditi-pandit · 2024-12-12T03:17:40Z

Thanks @bikramSingh91. Have made the changes. PTAL.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 28, 2024

aditi-pandit force-pushed the cve5 branch 3 times, most recently from b44c8ff to 1bd6cd3 Compare November 28, 2024 08:35

aditi-pandit marked this pull request as ready for review November 28, 2024 09:06

aditi-pandit requested a review from majetideepak as a code owner November 28, 2024 09:06

aditi-pandit force-pushed the cve5 branch 2 times, most recently from 09807e0 to f888c1a Compare November 28, 2024 15:33

aditi-pandit requested a review from czentgr November 28, 2024 15:42

czentgr approved these changes Nov 28, 2024

View reviewed changes

aditi-pandit commented Dec 2, 2024

View reviewed changes

aditi-pandit force-pushed the cve5 branch from f888c1a to 08ef7b3 Compare December 2, 2024 03:59

czentgr reviewed Dec 2, 2024

View reviewed changes

aditi-pandit force-pushed the cve5 branch 6 times, most recently from 9dde6f0 to 7b15200 Compare December 6, 2024 09:46

czentgr approved these changes Dec 6, 2024

View reviewed changes

aditi-pandit force-pushed the cve5 branch 2 times, most recently from 9cd2060 to 9bdf1cb Compare December 9, 2024 17:55

majetideepak reviewed Dec 10, 2024

View reviewed changes

velox/tpch/gen/dbgen/include/tpch_constants.hpp Outdated Show resolved Hide resolved

velox/tpch/gen/dbgen/speed_seed.cpp Outdated Show resolved Hide resolved

aditi-pandit force-pushed the cve5 branch 4 times, most recently from f57c4ae to 5f4a6ba Compare December 10, 2024 18:42

majetideepak approved these changes Dec 10, 2024

View reviewed changes

majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Dec 10, 2024

majetideepak reviewed Dec 10, 2024

View reviewed changes

aditi-pandit force-pushed the cve5 branch from 5f4a6ba to 97f04d9 Compare December 11, 2024 04:05

refactor: Change C style casts to C++ style (Part 5)

67c7ddb

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

aditi-pandit force-pushed the cve5 branch from f1a7029 to 67c7ddb Compare December 12, 2024 03:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Change C style casts to C++ style (Part 5) #11688

refactor: Change C style casts to C++ style (Part 5) #11688

aditi-pandit commented Nov 28, 2024

netlify bot commented Nov 28, 2024 •

edited

Loading

czentgr left a comment

aditi-pandit Dec 2, 2024

czentgr Dec 2, 2024

aditi-pandit Dec 3, 2024

majetideepak left a comment

majetideepak Dec 10, 2024

aditi-pandit Dec 11, 2024

facebook-github-bot commented Dec 10, 2024

majetideepak commented Dec 11, 2024

facebook-github-bot commented Dec 11, 2024

bikramSingh91 commented Dec 12, 2024

aditi-pandit commented Dec 12, 2024

refactor: Change C style casts to C++ style (Part 5) #11688

Are you sure you want to change the base?

refactor: Change C style casts to C++ style (Part 5) #11688

Conversation

aditi-pandit commented Nov 28, 2024

netlify bot commented Nov 28, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

czentgr left a comment

Choose a reason for hiding this comment

aditi-pandit Dec 2, 2024

Choose a reason for hiding this comment

czentgr Dec 2, 2024

Choose a reason for hiding this comment

aditi-pandit Dec 3, 2024

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

majetideepak Dec 10, 2024

Choose a reason for hiding this comment

aditi-pandit Dec 11, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Dec 10, 2024

majetideepak commented Dec 11, 2024

facebook-github-bot commented Dec 11, 2024

bikramSingh91 commented Dec 12, 2024

aditi-pandit commented Dec 12, 2024

netlify bot commented Nov 28, 2024 •

edited

Loading