Skip to content
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

Simplify overlap #4201

Merged
merged 2 commits into from
Sep 22, 2016
Merged

Simplify overlap #4201

merged 2 commits into from
Sep 22, 2016

Conversation

andralex
Copy link
Member

Related: http://forum.dlang.org/post/ner843$2cag$1@digitalmars.com but this is an improvement regardless of one's opinion of inout.

@schveiguy
Copy link
Member

schveiguy commented Apr 15, 2016

removing inout doesn't require using std.algorithm.max/min instead of internal versions. See same thread.

Edit: said that backwards. Using std.algorithm.min/max doesn't require removing inout.

static U* max(U* a, U* b) nothrow { return a > b ? a : b; }
static U* min(U* a, U* b) nothrow { return a < b ? a : b; }

import std.algorithm : min, max;
Copy link
Member

Choose a reason for hiding this comment

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

please add more detailed import

@andralex
Copy link
Member Author

@9il good point - done

@DmitryOlshansky
Copy link
Member

LGTM
@andralex to merge or not ? If inout is about to leave us, then let's merge.

@wilzbach
Copy link
Member

wilzbach commented May 2, 2016

@andralex to merge or not ? If inout is about to leave us, then let's merge.

From the discussion

I know of no "better" idioms, in terms of code generation and
specificity, to replace inout (or at least, inout as I envision it
should be). Care to elaborate?
I thought I did. Use one-liner functions that forward to templates.

Btw did anyone already point out that with inout the return type is inout(T)[] not auto?

@schveiguy
Copy link
Member

I don't want to say that this is extremely important to keep the way it is: we can remove inout from this function, and we pretty much gain nothing and lose compiler protection from mutation, but it's not super-harmful in this instance. I just think this is kind of a waste of time to make this minor change. If we are getting rid of inout, that decision hasn't been publicly announced, and we should at least wait until the path forward for those who depend on it is sorted.

Btw did anyone already point out that with inout the return type is inout(T)[] not auto?

You can change the inout version to return auto. Again, no need to remove inout.

@quickfur
Copy link
Member

quickfur commented May 3, 2016

We're killing inout? When was this decision made? Where's the forum thread / announcement?

@JackStouffer
Copy link
Member

We're killing inout? When was this decision made? Where's the forum thread / announcement?

@quickfur Andrei really wants to. See http://forum.dlang.org/thread/nepm2k$311l$1@digitalmars.com http://forum.dlang.org/thread/ner843$2cag$1@digitalmars.com and http://forum.dlang.org/post/n25qkc$2il8$1@digitalmars.com

@JackStouffer
Copy link
Member

JackStouffer commented Jul 15, 2016

I think this should be closed, due to the fact that the inout issue has been pretty much dropped.

@9il 9il added the @andralex label Jul 29, 2016
@9il
Copy link
Member

9il commented Jul 29, 2016

If we are getting rid of inout, that decision hasn't been publicly announced, and we should at least wait until the path forward for those who depend on it is sorted.

+1

@andralex
Copy link
Member Author

This simplifies code regardless of inout, so I guess it should be pulled. I'm recusing myself from reviewing it so I'll take out the label.

@PetarKirov
Copy link
Member

@andralex The min, max changes are good, but I'm against the change of signature. inout works well for what it is, so there's no need to replace it with an inferior solution. ref is the thing that needs fixing - being not part of the type it leads to ugly hacks and various limitations. If you disagree, please write a std.range.zip and std.range.enumerate alternatives that both 1) return ranges and 2) provide ref access usable in foreach and 3) without using string mixins. BTW, I think that even using string mixins wont help there.
Excuse me for the harsh tone, but I can't understand why you're trying to remove a useful feature without even a proper DIP supported by the community, whereas you're against any attempts to fixing ref on the grounds backwards compatibility (which would not be a problem in practice).
Removing inout without any alternative will put the D at disadvantage, whereas improving ref will make D more expressive and will enable designs not possible before - e.g. tuples supporting ref access and by extension the above-mentioned examples and many more.

@schveiguy
Copy link
Member

Can we separate the removal of inout from the obvious improvements of using std.algorithm.min/max? I think that PR could be pulled immediately.

@9il
Copy link
Member

9il commented Sep 22, 2016

We are spending to much efforts for this PR

@9il
Copy link
Member

9il commented Sep 22, 2016

Auto-merge toggled on

@9il 9il merged commit 4020810 into dlang:master Sep 22, 2016
@andralex
Copy link
Member Author

@9il well put thanks

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.

8 participants