Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

dmain uses Throwable.toString to print an exception/error #331

Closed
wants to merge 1 commit into from
Closed

dmain uses Throwable.toString to print an exception/error #331

wants to merge 1 commit into from

Conversation

Dav1dde
Copy link
Contributor

@Dav1dde Dav1dde commented Oct 21, 2012

This allows to actually override Throwable.toString and have a custom exception body, it also removes the code duplictation of printing the stacktrace.

@alexrp
Copy link
Member

alexrp commented Oct 21, 2012

This change may seem harmless, but there's a gotcha: Throwable.toString() uses the GC due to the way it concatenates strings, while the code here doesn't. I'm not sure if this is a problem in practice (but I think it can be if an OutOfMemoryError has been thrown).

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Oct 21, 2012

Like here?

Maybe branch of, if it's an OutOfMemoryError or any kind of Error and use the old way of printing the stacktrace? (There is this cast to Error anyways).

Pseudo:

auto e = cast(Error)t;
if(e is null) {
    // it's an exception
    console(t.toString());
} else {
   // do it the old way
}

@alexrp
Copy link
Member

alexrp commented Oct 21, 2012

@dawgfoto I think you know these mechanics of the runtime better than I do, can you weigh in? What's the best way we could do this such that all non-OOME errors use Throwable.toString()?

@complexmath
Copy link
Member

Switching to toString is fine as far as OutOfMemoryError is concerned--OOME overrides toString anyway. However, the reason for the current exception display approach is consistency. I'd originally used toString, and people complained that if they overrode toString they lost the formatting provided to built-in exceptions. The proper approach may be to change printMsgLine to print t.toString instead of t.msg and leave the rest in place.

@alexrp
Copy link
Member

alexrp commented Oct 26, 2012

@Dav1dde ping

@alexrp
Copy link
Member

alexrp commented Nov 18, 2012

Closing this. @Dav1dde feel free to open a new pull request with the aforementioned changes.

@alexrp alexrp closed this Nov 18, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants