Skip to content

Lower final switch default error to object.d template#7303

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

Lower final switch default error to object.d template#7303
dlang-bot merged 1 commit intodlang:masterfrom
somzzz:finalSwitchError

Conversation

@somzzz
Copy link
Contributor

@somzzz somzzz commented Nov 11, 2017

Druntime changes: dlang/druntime#1971

I still had to use SwitchErrorStatement because of the checks and actions the compiler performs for ErrorStatemenets vs regular Statements.

@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.

@somzzz somzzz force-pushed the finalSwitchError branch 3 times, most recently from 32bad72 to fb5b149 Compare November 11, 2017 15:06
Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Nits

RTLSYM_SWITCH_DSTRING,
RTLSYM_SWITCH_STRING, // unused
RTLSYM_SWITCH_USTRING, // unused
RTLSYM_SWITCH_DSTRING, // unused
Copy link
Contributor

Choose a reason for hiding this comment

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

https://issues.dlang.org/show_bug.cgi?id=9395 would be very useful here ;-)

src/ddmd/s2ir.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good style to provide an informative error message or do sth. like

assert(s.exp !is null, "SwitchErrorStatement needs to have a valid Expression.") 

and skip the if entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good style to provide an informative error message or do sth. like...

@WalterBright disagrees: #7079 (comment)

Some people like assert messages, I don't. They bloat up the compiler. Leaving the message as a comment in the source code is plenty good enough.

I really don't mind either way, just an FYI.

@somzzz somzzz force-pushed the finalSwitchError branch 2 times, most recently from b07c426 to a1dad09 Compare November 16, 2017 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments