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

add mixin template StandardExceptionConstructor #1413

Closed
wants to merge 2 commits into from
Closed

add mixin template StandardExceptionConstructor #1413

wants to merge 2 commits into from

Conversation

jamadagni
Copy link

Convenience mixin for trivially subclassing exceptions. Documentation and unittest included.

this(string msg, string file = __FILE__, size_t line = __LINE__)
{
super(msg, file, line);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that this is a waste of time, because you the constructors that get mixed in can't be documented, but if you're going to do do this, the correct constructors would be

this(string msg, string file = __FILE__, size_t line = __LINE__, Throwable next = null) @safe pure nothrow
{
    super(msg, file, line, next);
}

this(string msg, Throwable next, string file = __FILE__, size_t line = __LINE__) @safe pure nothrow
{
    super(msg, file, line, next);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the next parameter is not intended to be used from user code as you can see at: http://dlang.org/phobos/object.html#.Exception: "The next parameter is used internally and should always be null when passed by user code", and likewise the second overload is only needed for convenience of providing the next right after the message without having to manually include __FILE__ and __LINE__, so there's no point in including that overload either.

As for documentation, I thought using the mixin would actually enable it if you include a doc-comment block before it? Besides, if all you are doing is a trivial constructor for purposes of identification, then there should be no need for documentation since it's constructed in no way different than a regular exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I just tested it, and it looks like if you put a ddoc comment on the constructor, then that ddoc comment will actually show up in the exception's documentation. I did not think that that was the case. So, you're going to need to document the constructor(s) in the mixin.

As for next, every exception in druntime declares a constructor which takes it as do most of the Phobos exceptions. It's used for exception chaining. Please declare the constructors like so

/++
    Params:
        msg  = The message for the exception.
        file = The file where the exception occurred.
        line = The line number where the exception occurred.
        next = The previous exception in the chain of exceptions, if any.
  +/
this(string msg, string file = __FILE__, size_t line = __LINE__, Throwable next = null) @safe pure nothrow
{
    super(msg, file, line, next);
}

/++
    Params:
        msg  = The message for the exception.
        next = The previous exception in the chain of exceptions.
        file = The file where the exception occurred.
        line = The line number where the exception occurred.
  +/
this(string msg, Throwable next, string file = __FILE__, size_t line = __LINE__) @safe pure nothrow
{
    super(msg, file, line, next);
}

That is the standard way to do it, and the only exceptions that don't do that are ones which have alternate constructors, because they're not simply taking a message.

@jmdavis
Copy link
Member

jmdavis commented Oct 19, 2015

Also, this code has no business in object.d. It's a helper template, not something that needs to be pulled into every piece of D code ever. I'd have commented on that before, but I wasn't paying enough attention to where you put it. It should go in std.exception.

@jmdavis jmdavis closed this Oct 19, 2015
@jmdavis
Copy link
Member

jmdavis commented Oct 19, 2015

So, feel free to create a PR with this template mixin in std.exception, but I'm closing this PR, because it doesn't below here.

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.

2 participants