-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[opt](pointer cast) better typeid_cast #32614
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
Conversation
|
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
|
run buildall |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
TeamCity be ut coverage result: |
zclllyybb
left a comment
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.
LGTM
|
PR approved by anyone and no changes requested. |
| #else | ||
| if (typeid(*from) == typeid(std::remove_pointer_t<To>)) { | ||
| requires std::is_pointer_v<To> | ||
| To typeid_cast(From* from) noexcept { |
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.
In debug mode, the previous code will print exception. It is useful to help us debug.
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.
I dont know whether typeid_cast(nullptr) is designed as a common use scenario
| To typeid_cast(From* from) noexcept { | ||
| if ((typeid(From) == typeid(std::remove_pointer_t<To>)) || | ||
| (from && typeid(*from) == typeid(std::remove_pointer_t<To>))) | ||
| return static_cast<To>(from); |
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.
maybe we could always throw exception, not return nullptr.
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.
maybe we could always throw exception, not return nullptr.
doris/be/src/vec/columns/column_array.cpp
Line 513 in 2c01739
| if (typeid_cast<const ColumnUInt8*>(data.get())) |
the users of typeid_cast sometimes relay on the nullptr as their control flow, exception has bad performance when it is used as control flow.
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.
不是的,我们抛异常是在入参为空指针的时候才会抛。以前的行为,如果typeid不同,就是返回空指针。因为现在这个新写法应该不会带来更多的风险,应该是靠谱的。空指针的话肯定是代码有错才会出,这种情况下前、后的使用肯定会出问题,不必在这里处理成异常的。
bec8d22 to
eaa05d2
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
No description provided.