[
https://issues.apache.org/jira/browse/THRIFT-5678?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17679449#comment-17679449
]
Mario Emmenlauer commented on THRIFT-5678:
------------------------------------------
I'm currently travelling so I have limited internet access and could not check
the mentioned commit.
But my two cents: for the use case of thrift, I'd make all destructors virtual.
It comes at a (tiny) runtime cost, because virtual dispatch will be involved in
some method lookups. But in most cases, the cost of virtual dispatch is so
small that it can barely be measured (if you *really* want to know, for most
current CPUs the cost becomes only relevant when the virtual class table is
*not* in the CPU cache, so when the methods are rarely called. But in high
performance cases, the difference is not even measurable). At the same time,
virtual destructors brings safety in one specific use case: If user code
derives from thrift code, and deletes an object through a base class pointer,
there are memory leaks when the destructor is not virtual. Even if thrift
itself may be currently safe from that kind of deletion, such code may be added
in the future and then the problem can be easily overlooked, and even more,
user code may run into this issue.
Therefore, long story short, making all destructors virtual comes at almost
zero cost, but can bring a relevant benefit. It would be my choice of action.
> TConnectedClient: warning due to non-virtual dtor
> -------------------------------------------------
>
> Key: THRIFT-5678
> URL: https://issues.apache.org/jira/browse/THRIFT-5678
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
> Affects Versions: 0.13.0, 0.14.0, 0.15.0, 0.14.1, 0.14.2, 0.16.0, 0.17.0
> Reporter: Christopher Friedt
> Priority: Minor
> Time Spent: 10m
> Remaining Estimate: 0h
>
> In TConnectedClient.h the destructor is non-virtual, which causes the warning
> below.
> {code:java}
> In member function 'void
> apache::thrift::server::TServerFramework::disposeConnectedClient(apache::thrift::server::TConnectedClient*)':
> lib/cpp/src/thrift/server/TServerFramework.cpp:234:3: warning: deleting
> object of polymorphic class type 'apache::thrift::server::TConnectedClient'
> which has non-virtual destructor might cause undefined behavior
> [-Wdelete-non-virtual-dtor]
> 234 | delete pClient;
> | ^~~~~~~~~~~~~~{code}
> Not itself a major problem, but Zephyr CI promotes all warnings to errors,
> which causes builds to fail.
> TConnectedClient.cpp has the following line of code:
> {code:java}
> TConnectedClient::~TConnectedClient() = default;{code}
> It might be considered a regression. The commit that removed the virtual
> qualifier was 042580f53441efe1bc5c80c89351fcb30740659e
> Suggested fix is to mark it as virtual and set it to default in the header,
> and then delete the definition in the .cpp file.
> Alternatively, keep the .cpp dtor, but simply re-add the virtual keyword in
> the header
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
