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

Issue 14382 - converting old D1 operator overloading style to new D2 style #3130

Closed
wants to merge 5 commits into from

Conversation

schuetzm
Copy link
Contributor

@Safety0ff
Copy link
Contributor

// ///ditto
// VariantN opShr_r(T)(T lhs)
// {
// return VariantN(lhs).opLogic!(VariantN, ">>")(this);
// }
///ditto
VariantN opUShr(T)(T rhs) { return opLogic!(T, ">>>")(rhs); }
VariantN opBinary(string op : ">>>", T)(T rhs) { return opLogic!(T, ">>>")(rhs); }
Copy link
Member

Choose a reason for hiding this comment

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

All these functions are nearly identical. The whole point of having the template was so you can easily implement multiple operators with one set of code. For example:

VariantN opBinary(string op, T)(T rhs) if (op == "+" || op == "-" || op = "*" || op = "/")
{
    return opArithmetic!(T, op)(rhs);
}

In fact, I'd say get rid of opArithmetic and opLogic, and just move the implementations into an appropriate opBinary

@yebblies
Copy link
Member

yebblies commented Apr 1, 2015

This makes the code less readable. The old operator overloading methods aren't deprecated. The new methods are more powerful, and that power is wasted in 100% of these cases.

@schveiguy
Copy link
Member

This makes the code less readable.

What about with my suggested updates?

The old operator overloading methods aren't deprecated.

The old operator overloading methods are not documented (I will correct this), but should not be used in Phobos IMO. If there was ever a type that could use template + mixin opBinary, it would be Variant.

@DmitryOlshansky
Copy link
Member

Seems useful if @schveiguy suggestions are followed through. @schuetzm any plans to update this?

@schuetzm
Copy link
Contributor Author

This is ketmar's patch, I'm going to ask him. Maybe I'll also find some time to do it myself.

@JakobOvrum
Copy link
Member

@schuetzm, any news on this?

@quickfur
Copy link
Member

ping
Are we going ahead with this, or should we close it?

@schveiguy
Copy link
Member

@quickfur I don't think it's worth merging this unless we take advantage of the templated nature of the new operators. No sense in doing this without doing it right. And I don't think ketmar will do it, so if @schuetzm isn't, I think we can just close it.

@quickfur quickfur closed this Feb 20, 2016
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.

7 participants