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

fix Issue 15219 - Allow Throwable.msg to be calculated lazily #1411

Closed
wants to merge 1 commit into from

Conversation

CyberShadow
Copy link
Member

@CyberShadow
Copy link
Member Author

Ah, this PR found a bug already: std.csv.CSVException overrides toString, which is wrong: CSVException will not print a stack trace when it's thrown and not caught!

@CyberShadow
Copy link
Member Author

std.utf.UTFException also does this. I guess that explains the times I got "Invalid UTF sequence" with no stack trace.

@CyberShadow
Copy link
Member Author

It's tempting to mark Throwable.toString as final. It will break code, but it should only break broken code.

@CyberShadow
Copy link
Member Author

Actually, we should probably be moving towards something like a sink, for @nogc compliance. Hmm...

@jmdavis
Copy link
Member

jmdavis commented Oct 18, 2015

Well, I'm not sure that we've ever officially said that overriding toString on your exception class was not supposed to be allowed. If we were going to do that, we should have marked toString as final, and we didn't. I agree that it's not particularly good practice and that providing a custom message is what exceptions should be doing, but I don't know if it's reasonable to disallow it completely, much as I'd be tempted to agree.

As for the sink, @nogc compliance doesn't matter at the moment really when we're talking about exceptions. You have to allocate them anyway. The only time that you can get away with not allocating them is when you pre-allocate them, and that has its own problems. IMHO we should find a way to better support @nogc with exceptions, but that's probably going to require some language changes.

Also, Walter and Andrei have talked about moving away from toString to a function which returns a range of characters rather than having a toString that returns a string or one which takes an output range. So, that could definitely affect where we want to go with this, though I confess that I don't know how that's going to work with classes, since the toChars (or whatever it will be called) can't be templated if it's a virtual function, meaning that the return type is going to have to be a class, in which case, I don't know how we're going to avoid allocating it... In any case, the toString situation is definitely in limbo right now, so I don't know how much we want to mess with it at this point.

Similarly, msg should probably be changed in some way to use an input range rather than a string, but that would definitely be a breaking change, and doing anything like that without toString having been sorted out certainly makes no sense, particularly since it's more or less the same problem.

What you're proposing here seems like it should work just fine, but I don't know how it will jive with trying to moving away from string in APIs like this. However, I don't see how this change could make that harder later.

@andralex
Copy link
Member

shall we close this now?

@CyberShadow
Copy link
Member Author

@jmdavis There has been a lot of talk about making exceptions @nogc, which led to discussion about RC classes which in turn led to the lifetime study group. I haven't been following it so not sure what the progress is right now. AFAIU, currently exceptions are the biggest problem for making a lot of Phobos code @nogc.

@andralex Well, I opened this PR as a proof of concept and mostly for discussion. Yes, it could be improved as per above discussion. I don't know if that means it should be closed. Either way I would be really interested in hearing your opinion on the matter. Maybe this would be better discussed elsewhere, or maybe not - as too often happens, it could lead to discussions of grandiose redesigns which might never materialise when what is offered here is an improvement we can use now.

@jmdavis
Copy link
Member

jmdavis commented Jan 3, 2016

@CyberShadow I expect that Andrei is suggesting that we close this PR, because - for better or worse - #1445 was merged which does something similar - and in fact, it makes it so that msg should be deprecated at some point, since calling it is now risking bugs. Neither that change or this one really fit in well with the idea of rangifying toString and related functionality, but AFAIK, we haven't exactly figured out how we're going to do that yet (particularly when such functions can't be templated on classes - though maybe removing toString from Object like we're supposed to do at some point will give us an avenue around that problem).

And given the changes introduced with #1445, I do think that this PR should be closed, since they do almost the same thing and are essentially in conflict with one another. A wider discussion of how to deal with a rangified toString is probably justified as are discussions on how to fix it so that exceptions can be @nogc, but that belongs in the main newsgroup or in the new study list.

@jmdavis jmdavis closed this Jan 3, 2016
@CyberShadow
Copy link
Member Author

Hey, I only heard of #1445 now. I wish someone told me about it earlier.

But yes, it seems to be nearly identical, so I'm just fine with closing this then.

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