refactor: Change C style casts to C++ style (Part 3) #11686
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
aditi-pandit
changed the title
aditi-pandit
changed the title
| @@ -582,13 +583,15 @@ VectorPtr createStringFlatVectorFromUtf8View( | |||
| // buffer-index, 4-byte buffer-offset] to 16-byte Velox StringView [4-byte | |||
| // length, 4-byte prefix, 8-byte buffer-ptr] | |||
| for (int32_t idx_64 = 0; idx_64 < arrowArray.length; ++idx_64) { | |||
| auto* view = (uint32_t*)(&((uint64_t*)arrowArray.buffers[1])[2 * idx_64]); | |||
| rawStringViews[2 * idx_64] = *(uint64_t*)view; | |||
| auto* view = reinterpret_cast<const uint32_t*>(&( | |||
There was a problem hiding this comment.
Why add const here and elsewhere in the block instead of keeping it non-const?
Compared to line 783 change as well for example.
There was a problem hiding this comment.
arrwoArray.buffers is a const void** so the reinterpret_cast has error if we lose the constness. Same in other places where const is added.
velox/vector/arrow/Bridge.cpp
Outdated
| if (!vec.isNullAt(i) && view[0] > 12) { | ||
| uint64_t currAddr = *(uint64_t*)&view[2]; | ||
| uint64_t currAddr = *reinterpret_cast<const uint64_t*>(&view[2]); |
There was a problem hiding this comment.
Using const here isn't needed in this line? view being uint32* and then the value is immediately de-referenced.
aditi-pandit
force-pushed
the
cve3
branch
3 times, most recently
from
761ee2d to
5c5817e
Compare
|
@majetideepak @czentgr : There is one pending cast in Bridge.cpp:792 that I've not been able to resolve. Its a bit ugly because its overwriting the Velox StringView representation to Arrow StringView format and using some 64 bit to 32 bit pointer magic. It might need wider changes and I would prefer to handle that individually as it would need a very close inspection of the code and writing more tests. The changes in this PR are more straightforward changes in casts. |
There was a problem hiding this comment.
@aditi-pandit one comment.
The casts do look tricky here since we are deserializing nested arrow buffer contents.
Pointers are being generated from values.
But since we are only changing C casts to C++ casts, this should be safe.
velox/vector/arrow/Bridge.cpp
Outdated
| }); | ||
|
|
||
| auto utf8Views = (uint64_t*)out.buffers[1]; | ||
| auto utf8Views = reinterpret_cast<const uint64_t*>(out.buffers[1]); | ||
| int32_t bufferIdxCache = 0; | ||
| uint64_t bufferAddrCache = 0; | ||
|
|
||
| rows.apply([&](vector_size_t i) { | ||
| auto view = (uint32_t*)&utf8Views[2 * i]; |
There was a problem hiding this comment.
It should be okay to make this reinterpret_cast<const uint32_t*>(&utf8Views[2 * i])
There was a problem hiding this comment.
@majetideepak : Ah well... that isn't sufficient as the view pointer is used to modify contents around line 811. It can't be const.
view[2] = bufferIdxCache;
view[3] = currAddr - bufferAddrCache;
There was a problem hiding this comment.
Can we do const_cast<uint32_t*>(reinterpret_cast<const uint32_t*>(&utf8Views[2 * i]));?
There was a problem hiding this comment.
@majetideepak : There is also guidance to not cast away const https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es50-dont-cast-away-const
I was leaning to add a temporary buffer for StringView value modification from Velox format -> Arrow format in VeloxToArrowBridgeHolder and then initialize this just once into the ArrowStruct https://github.com/facebookincubator/velox/blob/main/velox/vector/arrow/Bridge.cpp#L43. That fits better with the class design.
But temporarily we can add const_cast if you are okay with that.
There was a problem hiding this comment.
The previous C-style cast is removing the const away anyway. I don't think this change will make anything worse.
The root cause seems to be that we use the same VeloxToArrowBridgeHolder class for import and export.
The following field must then be.
std::vector<const void*> buffers_{numBuffers_, nullptr}; -> std::vector<void*> buffers_{numBuffers_, nullptr};
aditi-pandit
force-pushed
the
cve3
branch
2 times, most recently
from
227b64b to
20bda29
Compare
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/vector
|
CI failure seems unrelated. |
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 12ae85b. |
…or#11686) 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/vector Pull Request resolved: facebookincubator#11686 Reviewed By: kagamiori Differential Revision: D66886326 Pulled By: bikramSingh91 fbshipit-source-id: dcece7a01bd880bfd12307d5829edc28057a049f
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/vector