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

Final switch on default error template#1971

Merged
dlang-bot merged 1 commit intodlang:masterfrom
somzzz:finalSwitchError
Nov 9, 2017
Merged

Final switch on default error template#1971
dlang-bot merged 1 commit intodlang:masterfrom
somzzz:finalSwitchError

Conversation

@somzzz
Copy link
Contributor

@somzzz somzzz commented Nov 6, 2017

Compiler lowers final switch default case to this (which is a runtime error).
Old implementation is in core/exception.d

Compiler changes: somzzz/dmd@d1bbf1e

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @somzzz! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@PetarKirov
Copy link
Member

This would be a breaking change. A better solution is to change onSwitchError like so:

@safe pure nothrow @nogc
void onSwitchError(string file = __FILE__, size_t line = __LINE__)
{
    throw staticError!SwitchError(file, line, null);
}

That way we keep compatibility with code that catches Error(s) (e.g. test runners, event loop / thread schedulers, etc.) and we make the implementation @nogc as well.
Also, exceptions provide better error messages.

@somzzz
Copy link
Contributor Author

somzzz commented Nov 9, 2017

@ZombineDev I didn't notice staticError, thanks for mentioning it!
It's not @nogc though.

src/core/exception.d(586): Error: @nogc function 'core.exception.__switch_errorT!().__switch_errorT' cannot call non-@nogc function 'core.exception.staticError!(SwitchError, string, ulong, typeof(null)).staticError'

Or after adding the @nogc tag to staticError:

src/core/exception.d(703): Error: @nogc function 'core.exception.staticError!(SwitchError, string, ulong, typeof(null)).staticError' cannot call non-@nogc constructor 'core.exception.SwitchError.this'

With the current solution, an AssertError is still thrown and dmd unittests pass. Still, it looks like onAssertError isn't @nogc either. https://github.com/dlang/druntime/blob/master/src/core/exception.d#L437

It shouldn't have been possible to mark code using final switch as @nogc in the first place.

@somzzz
Copy link
Contributor Author

somzzz commented Nov 9, 2017

Made SwitchError constructor @nogc and the rest was inferred. Similarly, __switch_errorT needs to be @trusted to call staticError.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Thanks for the update, now everything LGTM.

void __switch_errorT()(string file = __FILE__, size_t line = __LINE__) @trusted
{
// Consider making this a compile time check.
throw staticError!SwitchError(file, line, null);
Copy link
Member

@PetarKirov PetarKirov Nov 9, 2017

Choose a reason for hiding this comment

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

Keep in mind, that we may need to add a if (__ctfe) assert (0, "informative message...") branch, as I'm not sure if staticError would work in CTFE.
I prefer to leave the PR as it is for now, and cross the bridges when we come to them.

// Compiler lowers final switch default case to this (which is a runtime error)
void __switch_errorT()(string file = __FILE__, size_t line = __LINE__) @trusted
{
// Consider making this a compile time check.
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have that compile time check for final switch and normal switch always requires a default branch.

I guess this runtime check is only used to verify that a value is not out-of-range, probably caused by an invalid cast. Ping @andralex

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I didn't know final switch also works on non-enumeral values.

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.

5 participants

Comments