Skip to content

Comments

Merge enforceEx with enforce#2288

Merged
dnadlinger merged 1 commit intodlang:masterfrom
mihails-strasuns:enforce-unify
Jul 10, 2014
Merged

Merge enforceEx with enforce#2288
dnadlinger merged 1 commit intodlang:masterfrom
mihails-strasuns:enforce-unify

Conversation

@mihails-strasuns
Copy link

Makes it possible to call enforce!MyException instead of enforceEx!MyException, eliminating
redundant notation. Old enforceEx is not deprecated but removed from documentation examples.

This fixes one of very common Phobos "WAT" moments for me - having separate name for semantically identical
function with a different argument set.

Makes it possible to call `enforce!MyException` instead of `enforceEx!MyException`, eliminating
redundant notation. Old enforceEx is not deprecated but removed from documentation examples.
Copy link
Member

Choose a reason for hiding this comment

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

The patch looks very boring, I like it.

One question though, would their be any benefit of making the file and line template arguments like so:

T enforce(E : Throwable = Exception, T, string file = __FILE__, size_t line = __LINE__)(T value, lazy const(char)[] msg = null)

and passing it like this all the way to bailOut. I would image the compiler could take some advantage of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

No. It would only lead to a gazillion of duplicate enforce functions being created, and with longer mangled names.

Copy link
Member

Choose a reason for hiding this comment

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

thank you for the quick reply, I see my mistake

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we had it like this at one point, though, so it's not a completely outlandish suggestion. ;)

@quickfur
Copy link
Member

LGTM.

@dnadlinger
Copy link
Contributor

Auto-merge toggled on

@dnadlinger
Copy link
Contributor

Okay, let's go ahead with this. There might still be a small chance of breakage if somebody used enforce in an obscure way, but we'll find out during the next beta.

dnadlinger added a commit that referenced this pull request Jul 10, 2014
@dnadlinger dnadlinger merged commit 41421af into dlang:master Jul 10, 2014
@MartinNowak
Copy link
Member

Okay, let's go ahead with this. There might still be a small chance of breakage if somebody used enforce in an obscure way, but we'll find out during the next beta.

Anyone complained?
It would have been possible to add an overload for cases where someone explicitly named the return type.
Also why wasn't enforceEx deprecated?

@mihails-strasuns
Copy link
Author

Also why wasn't enforceEx deprecated?

I remember lot of negative opinions about breaking changes in Phobos for the sake of just naming - wanted to increase the chances of this PR to be merged :) I think we can try deprecating it in next release if no one is against it.

@mihails-strasuns
Copy link
Author

(I don't know of any complaints)

@mihails-strasuns mihails-strasuns deleted the enforce-unify branch October 14, 2014 02:52
@MartinNowak
Copy link
Member

Deprecation isn't breaking and we should still deprecate enforcers, starting by removing the documentation.

@MartinNowak
Copy link
Member

It would have been possible to add an overload for cases where someone explicitly named the return type.

What about the overload, this change as is might break code without warning.

@CyberShadow
Copy link
Member

This pull request introduced an accepts-invalid regression:
https://issues.dlang.org/show_bug.cgi?id=14685

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants