On Monday, 1 April 2013 at 23:03:56 UTC, Jonathan M Davis wrote:
On Monday, April 01, 2013 13:08:15 Lars T. Kyllingstad wrote:
It's time to clean up this mess.
http://wiki.dlang.org/DIP33
Another concern I have is InvalidArgumentError. There _are_
cases where it
makes sense for invalid arguments to be an error, but there are
also plenty of
cases where it should be an exception (TimeException is
frequently used in
that way), so we may or may not want an
InvalidArgumentException, but if you
do that, you run the risk of making it too easy to confuse the
two, thereby
causing nasty bugs. And most of the cases where
InvalidArgumentError makes
sense could simply be an assertion in an in contract, so I
don't know that
it's really needed or even particularly useful. In general, I
think that
having a variety of Exception types is valuable, because you
catch exceptions
based on their type, but with Errors, you're really not
supposed to catch
them, so having different Error types is of questionable value.
That doesn't
mean that we shouldn't ever do it, but they need a very good
reason to exist
given the relative lack of value that they add.
I definitely don't think we need an IllegalArgumentException.
IMO, passing illegal arguments is 1) a simple programming error,
in which case it should be an Error, or 2) something the
programmer can not avoid, in which case it requires a better
description of why things went wrong than just "illegal
argument". "File not found", for example.
I didn't really consider contracts when I wrote the DIP, and of
course there will always be the problem of "should this be a
contract or a normal input check?" The problem with contracts,
though, is that they go away in release mode, which is certainly
not safe if that is your error handling mechanism.
Also, if you're suggesting that these be _all_ of the exception
types in
Phobos, I don't agree. I think that there's definite value in
having specific
exception types for specific sections of code (e.g.
TimeException for time-
related code or CSVException in std.csv). It's just that they
should be put in
a proper place in the hierarchy so that users of the library
can choose to
catch either the base class or the derived class depending on
how specific
their error handling needs to be and on whatever else their
code is doing. We
_do_ want to move away from simply declaring module-specific
exception types,
but sometimes modules _should_ have specific exception types.
There may of course be, and probably are, a need for more
exceptions than what I've listed in the DIP. The idea was to
make a pattern, a system, to which more exceptions can be added
if strictly necessary. I do think, however, that we should try
to keep the number at a minimum, and that we should NOT create
new classes for every little detail that can go wrong.
The focus needs to be on creating a hierarchy that aids in
error handling, so
what exceptions we have should be based on what types of things
it makes sense
to catch in order to handle those errors specifically rather
than them being
treated as a general error, or even a general error of a
specific category.
Having a solid hierarchy is great and very much needed, but I
fear that your
DIP is too focused on getting rid of exception types rather
than shifting them
into their proper place in the exception hierarchy. In some
cases, that _does_
mean getting rid of exception types, but I think that on the
whole, it's more
of a case of creating new base classes for existing exceptions
so that we have
key base classes in the hierarchy. The DIP focuses on those
base classes but
seems to want to get rid of the rest, and I think that that's a
mistake.
It aims to get rid of the ones that don't add any value.
One more thing that I would point out is that your definition of
DocParseException won't fly. file and line already exist in
Exception with
different meanings, so you'd need different names for them in
DocParseException.
True.
Lars
