fixed Exception copy constructor, expanded what() #755
Conversation
|
Fix for issue #753 |
|
Note: there is a field in the Exception object for the stackTrace which is not implemented. It is a cool idea but hard to implement in all platforms/compilers. Lets leave that unimplemented for now. |
| Exception(filename_, lineno_, copy.getMessage(), stackTrace_); | ||
|
|
||
| Exception(const Exception ©) : std::runtime_error("") { | ||
| filename_ = copy.filename_; |
| @@ -158,6 +163,8 @@ class Exception : public std::runtime_error { //TODO rename to NTAException to b | |||
| UInt32 lineno_; | |||
| mutable std::string message_; //mutable bcs modified in getMessage which is used in what() but that needs be const | |||
| std::string stackTrace_; | |||
| mutable std::string what_; | |||
There was a problem hiding this comment.
do we need what_ to be a class member? it's not part of the constructors, and only used by what()
There was a problem hiding this comment.
Yes. The what() function is an override of the std::exception so we cannot change its signature. It returns a char*. This must point to something that will have a scope that last at least as long as the Exception class instance. If it is a local variable it goes away before it can be returned. So that is why I made it a class member.
| @@ -103,7 +104,11 @@ class Exception : public std::runtime_error { //TODO rename to NTAException to b | |||
| * @retval [const Byte *] the exception message | |||
| */ | |||
| virtual const char *what() const noexcept override { | |||
| return getMessage(); | |||
| try { | |||
| what_ = "Exception: " + Path::getBasename(filename_) + "(" + std::to_string(lineno_) + ") message: " + message_; | |||
There was a problem hiding this comment.
can you extend this for the logic in getMessage()? Otherwise, this won't properly work with streaming operators<<, I think.
There was a problem hiding this comment.
When 'what_' is computed the stream no longer exists. Let me explain...
When an exception is thrown, it is copied. Streams cannot be copied. So in the copy constructor, getMessage( ) is called. This will combine the streamed fields into message_ string and clear the stream. The new instance of Exception will have a new instance of the stringstream ss_ which will be empty. It is this copy of Exception that is referenced in the try/catch.
So, when "what_" is computed all of the streamed info is already in the message_. I could call getMessage( ) but it would just return message_.
There was a problem hiding this comment.
When an exception is thrown, it is copied. Streams cannot be copied. So in the copy constructor, getMessage( ) is called.
thanks for this explanation! 👍
| NTA_THROW << "This msg"; | ||
| } catch (const Exception &e) { | ||
| EXPECT_STREQ(e.getMessage(), "This msg"); | ||
| EXPECT_STREQ(e.what(), "Exception: ExceptionTest.cpp(52) message: This msg"); |
There was a problem hiding this comment.
ok here, but try e.what() before the e.getMessage(), see my comment above.
There was a problem hiding this comment.
The order will make no difference because e.getMessage( ) was already called once in the copy constructor so message_ already contains the streamed data as explained above.
Oh, and I did try both ways to confirm.
There was a problem hiding this comment.
I just discovered a situation where the copy would not occur.
If the Exception object in the try block ends up on the stack in the same place in the catch block then the copy does not occur.
This happens in the other two test cases which I had not tried until this morning. If the NTA_THROW macro is used, it is inside an 'if' block so its stack location changed thus it forces the copy, but not in the other two test cases.
So, you are right, I still need the call to getMessage( ) when creating what_.
You already merged so I will include that change in my next PR which is about ready to be pushed.
While fixing the copy constructor on the Exception object I expended the what function to also include the source file and line number with the error message.
I added a unit test in htm/types/ExceptionTest.cpp