Remove a catch of Throwable in object.d#1743
Conversation
| } | ||
| catch (Throwable) | ||
| catch (Exception) | ||
| { |
There was a problem hiding this comment.
Is it even at any time a good idea to silently catch Exceptions? It seems to me a bit suspicious that toString doesn't expose the nothrow guarantee publicly, but in the other hand does this silent catching..
There was a problem hiding this comment.
Exception implies that the program is still in a recoverable state. Errors imply some unrecoverable state.
There was a problem hiding this comment.
Yeah I know, but I was trying to say that silently swallowing them as it is done in the existing code isn't really a good practice.
JackStouffer
changed the title
|
Had to remove the second change as it's error-ing and I don't have time to debug right now. |
|
Pinging people with merge rights |
src/object.d
Outdated
| return cast(size_t)cast(void*)this; | ||
| } | ||
| auto data = this.toString(); | ||
| return hashOf(data, 0); |
There was a problem hiding this comment.
I know that it was like this before, but is there any reason to create a variable here? It seems like a pointless extra line.
|
@jmdavis Fixed |
|
Just wasted time looking at this, because github doesn't send me notifications of labels being added. Grrr... |
|
5 participants
Catching
Throwableis a bad idea most of the time.In the second case, there's no obvious reason why we would want to catchErrors from user code generated by theTraceInfo.opApply