Simplify C++ exceptions #3380
Simplify C++ exceptions #3380
Conversation
bernardnormier
requested review from
pepone,
externl and
InsertCreativityHere
| @@ -1,9 +1,513 @@ | |||
| // Copyright (c) ZeroC, Inc. | |||
|
|
|||
| #if defined(_MSC_VER) | |||
There was a problem hiding this comment.
This code was largely moved from Exception.cpp. In particular, there was no change to the stack trace capture logic.
| } | ||
|
|
||
| ostream& | ||
| Ice::operator<<(ostream& out, const Ice::Exception& ex) |
There was a problem hiding this comment.
This PR eliminates the distinction ice_print / operator<<, which made this code too complicated.
As of this PR, (virtual) ice_print and operator<< are the same.
| Exception(const Exception&) = default; | ||
| Exception& operator=(const Exception&) = default; | ||
|
|
||
| // Need out-of-line virtual function to avoid weak vtable, which in turn requires the default constructor, |
There was a problem hiding this comment.
Weak vtables break dynamic_cast and catch(const XxxException&), and it's typically hard to find since we see these errors only at runtime. This particular weak vtable breaks the Ice/background test when it catches const Ice::Exception& on macos.
| void | ||
| Ice::UserException::ice_print(ostream& os) const | ||
| { | ||
| os << ice_id(); |
There was a problem hiding this comment.
This implementation will change in a follow-up PR. See #3378.
| } | ||
|
|
||
| // | ||
| // Test operator<<(). |
There was a problem hiding this comment.
No need to test operator<< since it's now identical to ice_print.
| Ice::LocalException::LocalException(const char* file, int line, string message) | ||
| : _file(file), | ||
| _line(line), | ||
| _whatString(make_shared<string>(std::move(message))), |
There was a problem hiding this comment.
Not related to the PR, should we use unique_ptr for whatString. There is no shared ownership here.
There was a problem hiding this comment.
No, we have to use a shared_ptr. Same for other exceptions with string and similar fields.
This is to ensure the copy ctor of the exception is noexcept.
| @@ -152,13 +152,6 @@ namespace | |||
| unknownExceptionMessage = createUnknownExceptionMessage(exceptionId, ex.what()); | |||
| replyStatus = ReplyStatus::UnknownLocalException; | |||
| } | |||
| catch (const Exception& ex) | |||
There was a problem hiding this comment.
We don't see this catch block since we already catch UserException and LocalException above.
| Ice::LocalException::LocalException(const char* file, int line, string message) | ||
| : _file(file), | ||
| _line(line), | ||
| _whatString(make_shared<string>(std::move(message))), |
There was a problem hiding this comment.
No, we have to use a shared_ptr. Same for other exceptions with string and similar fields.
This is to ensure the copy ctor of the exception is noexcept.
| /// @param message The error message adopted by this exception and returned by what(). | ||
| LocalException(const char* file, int line, std::string message); | ||
|
|
||
| [[nodiscard]] const char* what() const noexcept final; |
There was a problem hiding this comment.
I know that what is pretty standard C++ exception stuff, but should we carry over the doc-comment for it?
/**
* Returns the error message of this exception.
* @return The error message.
*/
[[nodiscard]] const char* what() const noexcept override;
| // +2 to remove the leading "::" from the type ID. | ||
| os << ice_id() + 2 << ' ' << what(); |
There was a problem hiding this comment.
I didn't know you could do that.
What a crazy syntax.
This PR simplifies exceptions in C++ and updates the base classes as follows:
Ice::Exception
The common base class, provides what is truly common between local exceptions and user exceptions, namely:
ice_print(ostream&) const, used by the implementation ofIce::operator<<(ostream&, const Exception&)ice_id(), the Slice type ID or a similar string for local exceptionsIt has only 2 derived classes, LocalException and UserException (this didn't change).
Ice::LocalException
The base class for Ice exceptions not defined in Slice. It's a true std::exception, with a custom what() message. It also holds where the exception was thrown (file + line) and can capture / provide its stack trace.
This PR moves all the custom what / stack trace code from Ice::Exception to Ice::LocalException.
Ice::UserException
The base class for Slice-defined exceptions. User exceptions are primary class-like errors transmitted over the wire. The exception-aspect is secondary. In particular, we don't capture the stack trace nor provide a custom
whatmessage.what()is always the same as the Slice type ID (returned byice_id()), and is typically never called.