New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

away with the boilerplate of Exception subclasseses #622

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@MartinNowak
Member

MartinNowak commented Oct 3, 2013

No description provided.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Oct 3, 2013

Member

Thanks @9rnsr for the quick fix to __MODULE__ as default template parameter dlang/dmd#2617.

Member

MartinNowak commented Oct 3, 2013

Thanks @9rnsr for the quick fix to __MODULE__ as default template parameter dlang/dmd#2617.

@nazriel

This comment has been minimized.

Show comment
Hide comment
@nazriel

nazriel Oct 3, 2013

Awesome pull request 👍

nazriel commented Oct 3, 2013

Awesome pull request 👍

@JakobOvrum

This comment has been minimized.

Show comment
Hide comment
@JakobOvrum

JakobOvrum Oct 3, 2013

Member

I much prefer the mixin template approach to the alias approach of fixing this issue.

  • First of all, whenever the exception needs anything that deviates from the provided template, such as a non-Exception base type, data fields or a custom constructor, the programmer is left no choice but to use the old style of defining exceptions, including all the boilerplate we aim to eliminate.
  • Using a mixin template is conducive to writing good, well-thought-out exception types, while this approach puts a major hurdle in the way instead, as any exception that doesn't fit the ExceptionImpl template can only be crafted with lots of boilerplate work.
  • Using alias means repeating the name of the exception twice, with no guarantee that the string repetition matches the actual name of the exception. Maintenance pains ensue.
  • Documentation looks different for an alias and a class, and with changing requirements as elaborated on above, it could change back and forth between versions.

Please reconsider.

Member

JakobOvrum commented Oct 3, 2013

I much prefer the mixin template approach to the alias approach of fixing this issue.

  • First of all, whenever the exception needs anything that deviates from the provided template, such as a non-Exception base type, data fields or a custom constructor, the programmer is left no choice but to use the old style of defining exceptions, including all the boilerplate we aim to eliminate.
  • Using a mixin template is conducive to writing good, well-thought-out exception types, while this approach puts a major hurdle in the way instead, as any exception that doesn't fit the ExceptionImpl template can only be crafted with lots of boilerplate work.
  • Using alias means repeating the name of the exception twice, with no guarantee that the string repetition matches the actual name of the exception. Maintenance pains ensue.
  • Documentation looks different for an alias and a class, and with changing requirements as elaborated on above, it could change back and forth between versions.

Please reconsider.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 3, 2013

Ultimately the issue is caused by the lack of constructor inhertiance. C++11 has this feature, so why not introduce it in D?

ghost commented Oct 3, 2013

Ultimately the issue is caused by the lack of constructor inhertiance. C++11 has this feature, so why not introduce it in D?

@nazriel

This comment has been minimized.

Show comment
Hide comment
@nazriel

nazriel Oct 3, 2013

Actually I think @JakobOvrum is right.

alias FiberException = ExceptionImpl!"FiberException";

vs

class FiberException : Exception { mixin ExceptionImpl!(); }

A bit more typing in Jakob's case but gives way much more flexibility.
Could be useful and reusable in users code too.

nazriel commented Oct 3, 2013

Actually I think @JakobOvrum is right.

alias FiberException = ExceptionImpl!"FiberException";

vs

class FiberException : Exception { mixin ExceptionImpl!(); }

A bit more typing in Jakob's case but gives way much more flexibility.
Could be useful and reusable in users code too.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Oct 3, 2013

Member

I also considered a mixin template but I don't think they are a good fit here.
This is intended as a no frills helper for the idiom of throwing specialized types
per error kind.

Mixing arbitrary functions into your class definition comes with macro style pain,
e.g. changing the macro would easily break client code.
Also the implementation of such Exceptions is no longer self contained which
encourages to write broken code like this.

class MyException : Exception
{
    mixin ExceptionImpl!();
    this (uint num) { super(...); }
}

First of all, whenever the exception needs anything that deviates from the provided template, such as a non-Exception base type, data fields or a custom constructor, the programmer is left no choice but to use the old style of defining exceptions, including all the boilerplate we aim to eliminate.

If you need anything that's non-standard you most likely need a different constructor.

Using alias means repeating the name of the exception twice, with no guarantee that the string repetition matches the actual name of the exception. Maintenance pains ensue.

Yes twice, but the name is only used for printing and you're exaggerating the maintenance part here.

Documentation looks different for an alias and a class, and with changing requirements as elaborated on above, it could change back and forth between versions.

True, but alias TimeException = core.exception.ExceptionImpl!"TimeException" is pretty self-documenting.

Ultimately the issue is caused by the lack of constructor inhertiance. C++11 has this feature, so why not introduce it in D?

Yes, please implement. We already have constructor inheritance for default constructor so I don't think anybody would oppose that. It's a superior solution to this problem.

Member

MartinNowak commented Oct 3, 2013

I also considered a mixin template but I don't think they are a good fit here.
This is intended as a no frills helper for the idiom of throwing specialized types
per error kind.

Mixing arbitrary functions into your class definition comes with macro style pain,
e.g. changing the macro would easily break client code.
Also the implementation of such Exceptions is no longer self contained which
encourages to write broken code like this.

class MyException : Exception
{
    mixin ExceptionImpl!();
    this (uint num) { super(...); }
}

First of all, whenever the exception needs anything that deviates from the provided template, such as a non-Exception base type, data fields or a custom constructor, the programmer is left no choice but to use the old style of defining exceptions, including all the boilerplate we aim to eliminate.

If you need anything that's non-standard you most likely need a different constructor.

Using alias means repeating the name of the exception twice, with no guarantee that the string repetition matches the actual name of the exception. Maintenance pains ensue.

Yes twice, but the name is only used for printing and you're exaggerating the maintenance part here.

Documentation looks different for an alias and a class, and with changing requirements as elaborated on above, it could change back and forth between versions.

True, but alias TimeException = core.exception.ExceptionImpl!"TimeException" is pretty self-documenting.

Ultimately the issue is caused by the lack of constructor inhertiance. C++11 has this feature, so why not introduce it in D?

Yes, please implement. We already have constructor inheritance for default constructor so I don't think anybody would oppose that. It's a superior solution to this problem.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 3, 2013

Yes, please implement.

I think the only real issue is what syntax to use. C++ uses:

using Base::Base;

At first glance in D it would be:

alias Base.this this;  // or Super.this

But this conflicts with subtyping syntax.

ghost commented Oct 3, 2013

Yes, please implement.

I think the only real issue is what syntax to use. C++ uses:

using Base::Base;

At first glance in D it would be:

alias Base.this this;  // or Super.this

But this conflicts with subtyping syntax.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Oct 3, 2013

Member

@AndrejMitrovic I replied to the constructor inheritance in the enhancement request Issue 9066.

Member

MartinNowak commented Oct 3, 2013

@AndrejMitrovic I replied to the constructor inheritance in the enhancement request Issue 9066.

@jmdavis

This comment has been minimized.

Show comment
Hide comment
@jmdavis

jmdavis Oct 4, 2013

Member

If we don't have constructor inheritance, I think that this approach is superior to the mixin, because the constructors declared here likely shouldn't be declared if you need any extra members. So, the mixin becomes nearly useless outside of this exact use case. As such, I don't really see any benefit of it over this.

Member

jmdavis commented Oct 4, 2013

If we don't have constructor inheritance, I think that this approach is superior to the mixin, because the constructors declared here likely shouldn't be declared if you need any extra members. So, the mixin becomes nearly useless outside of this exact use case. As such, I don't really see any benefit of it over this.

@JakobOvrum

This comment has been minimized.

Show comment
Hide comment
@JakobOvrum

JakobOvrum Oct 4, 2013

Member

This is intended as a no frills helper for the idiom of throwing specialized types
per error kind.

And it's going to encourage people to always do that instead of putting more thought into their exception types.

e.g. changing the macro would easily break client code.

The documentation must be clear about what it mixes in. Changing what it mixes in is a breaking change like any other, it's not a particularity of being a "macro".

Also the implementation of such Exceptions is no longer self contained which
encourages to write broken code like this.

I don't see any reason to consider code like that broken; users want what they want. The same code is possible with constructor inheritance.

Member

JakobOvrum commented Oct 4, 2013

This is intended as a no frills helper for the idiom of throwing specialized types
per error kind.

And it's going to encourage people to always do that instead of putting more thought into their exception types.

e.g. changing the macro would easily break client code.

The documentation must be clear about what it mixes in. Changing what it mixes in is a breaking change like any other, it's not a particularity of being a "macro".

Also the implementation of such Exceptions is no longer self contained which
encourages to write broken code like this.

I don't see any reason to consider code like that broken; users want what they want. The same code is possible with constructor inheritance.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Oct 5, 2013

Member

If we don't have constructor inheritance, I think that this approach is superior to the mixin, because the constructors declared here likely shouldn't be declared if you need any extra members. So, the mixin becomes nearly useless outside of this exact use case. As such, I don't really see any benefit of it over this.

Exactly.

The documentation must be clear about what it mixes in. Changing what it mixes in is a breaking change like any other, it's not a particularity of being a "macro".

It's more problematic because as a library writer you have no control/knowledge about other members in the same scope which might cause unwanted interference.

I don't see any reason to consider code like that broken; users want what they want.

It's broken because it allows to construct such an exception without using the constructor that initialized the additional members.

The same code is possible with constructor inheritance.

Nope, if you define your own constructor none is implicitly inherited.

Member

MartinNowak commented Oct 5, 2013

If we don't have constructor inheritance, I think that this approach is superior to the mixin, because the constructors declared here likely shouldn't be declared if you need any extra members. So, the mixin becomes nearly useless outside of this exact use case. As such, I don't really see any benefit of it over this.

Exactly.

The documentation must be clear about what it mixes in. Changing what it mixes in is a breaking change like any other, it's not a particularity of being a "macro".

It's more problematic because as a library writer you have no control/knowledge about other members in the same scope which might cause unwanted interference.

I don't see any reason to consider code like that broken; users want what they want.

It's broken because it allows to construct such an exception without using the constructor that initialized the additional members.

The same code is possible with constructor inheritance.

Nope, if you define your own constructor none is implicitly inherited.

@JakobOvrum

This comment has been minimized.

Show comment
Hide comment
@JakobOvrum

JakobOvrum Oct 7, 2013

Member

It's broken because it allows to construct such an exception without using the constructor that initialized the additional members.

I thought you were referring to the fact that the new constructor didn't use __FILE__ and __LINE__.

I don't consider not allowing any exception tailoring at all to be a solution to exception tailoring that results in buggy code. If that was a solution, there'd be no reason to have throwables be a class hierarchy at all.

Member

JakobOvrum commented Oct 7, 2013

It's broken because it allows to construct such an exception without using the constructor that initialized the additional members.

I thought you were referring to the fact that the new constructor didn't use __FILE__ and __LINE__.

I don't consider not allowing any exception tailoring at all to be a solution to exception tailoring that results in buggy code. If that was a solution, there'd be no reason to have throwables be a class hierarchy at all.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Oct 7, 2013

Member

I don't consider not allowing any exception tailoring at all to be a solution to exception tailoring that results in buggy code. If that was a solution, there'd be no reason to have throwables be a class hierarchy at all.

This is just a polemic not an argument.

The pull request simplifies a common idiom. You can look at phobos to see that many exceptions would benefit from this.
And still a mixin is of little to no use for exception tailoring.

Member

MartinNowak commented Oct 7, 2013

I don't consider not allowing any exception tailoring at all to be a solution to exception tailoring that results in buggy code. If that was a solution, there'd be no reason to have throwables be a class hierarchy at all.

This is just a polemic not an argument.

The pull request simplifies a common idiom. You can look at phobos to see that many exceptions would benefit from this.
And still a mixin is of little to no use for exception tailoring.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Oct 11, 2013

Member

Seems like we cannot reach a consensus here and I don't have a nice solution to preserve the documentation either.
I'll change this pull to contain only the dmain refactoring and will defer the exception boilerplate to implicif constructor inheritance.

Member

MartinNowak commented Oct 11, 2013

Seems like we cannot reach a consensus here and I don't have a nice solution to preserve the documentation either.
I'll change this pull to contain only the dmain refactoring and will defer the exception boilerplate to implicif constructor inheritance.

MartinNowak added some commits Oct 11, 2013

use Throwable.toString to report unhandled exceptions
- Add toString(sink) overload to Throwable, so that
  it can be used without the GC.

- Rewrite string toString() in terms of the sink overload.
  Thus toString(sink) becomes the preferred method to
  override in derived Throwables.
@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Oct 11, 2013

Member

I opened #636 for the Throwable.toString(sink) part.

Member

MartinNowak commented Oct 11, 2013

I opened #636 for the Throwable.toString(sink) part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment