On Friday, 24 March 2017 at 19:38:14 UTC, H. S. Teoh wrote:
Catching an Exception by message? That sounds like horrible
code smell to me.
Yes, it is. That's why I think exception strings are an
antipattern. You should be making new classes, not new strings.
But, D lets us have both worlds. Consider the following:
---
class MallocedException : Exception {
@nogc
this(string msg, string file = __FILE__, size_t line = __LINE__,
Throwable next = null) {
super(msg, file, line, next);
}
@nogc
private void freeSelf() {
import core.stdc.stdlib;
import core.stdc.stdio;
printf("freeing\n");
free(cast(void*) this);
}
~this() {
freeSelf();
}
}
class RaisedException(string msg) : MallocedException {
@nogc
this(string file = __FILE__, size_t line = __LINE__, Throwable
next = null) {
super(msg, file, line, next);
}
}
class RaisedExceptionDetails(string message, T...) :
RaisedException!message {
T args;
@nogc
this(T args, string file = __FILE__, size_t line = __LINE__,
Throwable next = null) {
this.args = args;
super(file, line, next);
}
override void toString(scope void delegate(in char[]) sink)
const {
import core.internal.traits : externDFunc;
alias sizeToTempString =
externDFunc!("core.internal.string.unsignedToTempString",
char[] function(ulong, char[], uint) @safe pure
nothrow @nogc);
char[20] tmpBuff = void;
sink("RaisedException");
sink("@"); sink(file);
sink("("); sink(sizeToTempString(line, tmpBuff, 10)); sink(")");
if (message.length)
{
sink(": "); sink(message);
}
foreach(idx, arg; args) {
sink("\n\t[");
sink(sizeToTempString(idx, tmpBuff, 10));
sink("] = ");
static if(is(typeof(arg) : const(char)[]))
sink(arg);
else static if(is(typeof(arg) : long))
sink(sizeToTempString(arg, tmpBuff, 10));
else
{} // FIXME support more
}
if (info)
{
try
{
sink("\n----------------");
foreach (t; info)
{
sink("\n"); sink(t);
}
}
catch (Throwable)
{
// ignore more errors
}
}
}
}
@trusted @nogc
void raise(string msg, string file = __FILE__, size_t line =
__LINE__, T...)(T args) {
import core.stdc.stdlib, std.conv;
enum size = __traits(classInstanceSize,
RaisedExceptionDetails!(msg, T));
void* buffer = malloc(size);
assert(buffer !is null);
throw emplace!(RaisedExceptionDetails!(msg, T))(buffer[0 ..
size], args, file, line);
}
void main() {
raise!"my_exception"(12, "additional info", 32);
}
---
That raise line lets you define your string if you must, as well
as *any* other data you want to pass along, with a usable, if a
bit less than ideal, toString representation of it.
BTW, I also went ahead and made this @nogc to show that's not
really so hard to do. If you catch a MallocException, just
.destroy(it). Easy (at least if you control both sides of the
code).
Notice that there are also no Phobos imports and no user
allocations at the raise site.
The only thing I'd add to it is some kind of
raise!YourExceptionBase overload and I'd find this pretty useful.
It probably makes sense for 3rd party libraries to have at
least a subclass of Exception, so that you can catch errors
originating from that library rather than everything in general.
I tend to do very broad classifications. My script.d, for
example, can throw three supertypes of exception: compile
exception, runtime exception, and user-defined exceptions out of
the script.
Those are the things you'd likely catch, because each one has a
different yet reasonable possible response.
But, if additional info is easy to attach, I'd go ahead and do
that too - there's just no reason not to when it is easy, and the
catcher can then decide to get into more details if interested,
or just catch one of the super classes if not.
Actually, I see this as evidence *against* having RangeError in
the first place. If we had stuck to throwing Exception or, in
this case, Error, that would have prompted whoever wrote the
bounds check code to actually write a useful error message
Come on, be realistic. We both know it would be `throw new
Exception("RangeError");` with no additional data.
In fact, since building strings is *significantly* more difficult
than just passing arguments, building a useful string is expected
to be uncommon... and it is. Grep phobos and find the majority of
exception strings are static literals, even when more information
could be available.
Except that it's not really *that* useful if you don't know
what those numbers mean.
The class toString could also label them trivially, or, of
course, you could document it for those who care (and those who
don't care will just learn to ignore the noise).
It is far more likely that the class toString would give useful
results than throw new Exception(format(...)) since:
1) it is written once, at the class definition (or in the mixin
reflection template), instead of at every throw point
and 2) functions like format("") aren't available in a LOT of
contexts, whereas toString(sink) from data internal to the
exception can be done virtually anywhere. format("") adds a
Phobos dependency, making it a no go for druntime, it adds a GC
dependency, making it no go in those nogc contexts, and having to
add the import can be as much of a pain as importing a exception
helper template anyway.
But even if it isn't labeled, you could look it up, or get used
to it, or maybe even guess it from context. The point is that the
info is THERE for those who want to read it. (Frankly, I prefer
just segfaulting than throwing the useless RangeError as it is
now, at least a core dump can be loaded up in a debugger.)
A similar discussion came up some years ago, where Andrei
proposed adding a Variant[string] field to Exception where the
thrower can stick whatever details he likes into it.
Yeah, that's useless, it has all the same problems as format(),
plus some.
It's already kind enough on my part to actually want to use
std.format to produce a message that a human reader might find
helpful, like "syntax error on line 36: unexpected character
'x'". I could have just done a `throw new Exception("syntax
error");` and be done with it. At least I, the human, would
read that message if the Exception got thrown, but put in
additional effort so that it's parseable by code?
Reminder: we're using D here. It is LESS effort to add info than
it is to call format().
