refactor: Change C style casts to C++ style (Part 4) #11687
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
aditi-pandit
changed the title
|
Test failures related to #11721 mostly. Will rebase after that PR is submit. |
aditi-pandit
force-pushed
the
cve4
branch
2 times, most recently
from
ed180ee to
01e404f
Compare
| sumC_ += (double)count; | ||
| sumCLogC_ += (double)count * std::log(count); | ||
| sumC_ += static_cast<double>(count); | ||
| sumCLogC_ += static_cast<double>(count * std::log(count)); |
There was a problem hiding this comment.
This should be: static_cast<double>(count) * std::log(count);
There was a problem hiding this comment.
Nice catch. Updated.
| @@ -154,7 +154,7 @@ class ArrayWriterBenchmark : public functions::test::FunctionBenchmarkBase { | |||
| } | |||
|
|
|||
| vector_size_t size = 1'000; | |||
| size_t totalItemsCount = (size) * (size + 1) / 2; | |||
| size_t totalItemsCount = static_cast<size_t>((size) * (size + 1) / 2); | |||
There was a problem hiding this comment.
I guess I have some questions around vector_size_t being int32_t. A vector access via [] operator or at would use a size_type parameter which is an unsigned integral type.
And the Velox vectors should work similar to std::vector in terms of access.
If it was unsigned then we could use implicit type conversion which still would work here I think.
There was a problem hiding this comment.
I'm not 100% sure but I generally thought it was that way to keep the sizes of vectors bounded in Velox. But yes, being consistent with std::vector access is a good idea.
Maybe open a discussion in Velox about it ?
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/functions
majetideepak
added
the
ready-to-merge
|
@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 109ef3b. |
…or#11687) Summary: 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/functions Pull Request resolved: facebookincubator#11687 Reviewed By: Yuhta Differential Revision: D67060300 Pulled By: bikramSingh91 fbshipit-source-id: e127c7d36c3ef9248d2eb8fe0ae85d8f065871e6
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/functions