Skip to content

Supplemental changes of dmd/pull/41 #120

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

Merged
merged 1 commit into from
Feb 21, 2012

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Jun 24, 2011

dlang/dmd#41 makes struct literal and struct constructor call as rvalue, and passing them to ref parameter make invalid.
This pull request adds function overloads for ref and non-ref.

CAUTION: It is necessary to apply changes to phobos and dmd at the same time.

@@ -335,7 +335,7 @@ public:
}

///
bool opEquals(Tdummy=void)(ref const BigInt y) const
bool opEquals(Tdummy=void)(auto ref const BigInt y) const
Copy link
Member

Choose a reason for hiding this comment

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

As I understand it, you don't actually need the Tdummy=void. Supposedly, () is enough to templatize the function (I haven't tried it though; I just know that David was doing it in some code). Assuming that that works, it would likely be preferable to having Tdummy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late reply. I had missed your comment.

I also think that Tdummy=void is not needed. It might be the garbage of past template limitation.
I couldn't judge it, so keep codes as far as possible.

@andralex
Copy link
Member

andralex commented Sep 4, 2011

Ping? Please rebase so I can pull safely. Thanks!

@9rnsr
Copy link
Contributor Author

9rnsr commented Sep 4, 2011

Unfortunately, we can't merge this until merging dmd/pull/41.
Because current dmd has a bug that the function overloading ref and non-ref does not work.

  bool opEquals(ref const SList rhs) const { ... }
+ bool opEquals(const SList rhs) const { ... }  // this addition reveals the dmd bug

Then, it is necessary to apply changes to phobos and dmd at the same time.

@andralex
Copy link
Member

andralex commented Sep 4, 2011

It would be great to fix this issue in the coming release. Kenji, could you please submit the bug? Walter, any chance we get a fix in the current beta? Thanks!

@9rnsr
Copy link
Contributor Author

9rnsr commented Sep 5, 2011

I have already filed the bug as bug 5889, but Walter has a negative opinion against fixing it...

@andralex
Copy link
Member

@k-hara: This pull as is right now can be merged independently of dmd changes?

@andralex
Copy link
Member

I mean @9rnsr :o)

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2012

@andralex: Still can't, unfortunately. We MUST merge dmd/pull/41 before this, to avoid conflict of ref/non-ref overload.

@andralex
Copy link
Member

@WalterBright: ping

WalterBright added a commit that referenced this pull request Feb 21, 2012
Supplemental changes of dmd/pull/41
@WalterBright WalterBright merged commit d780d9d into dlang:master Feb 21, 2012
@WalterBright
Copy link
Member

Please also fix opEquals() to be @safe or @trusted.

@WalterBright
Copy link
Member

Fails miserably at compiling phobos.

@WalterBright
Copy link
Member

std\file.d(2103): Error: overloads const pure nothrow bool(const(SysTime) rhs) and const pure nothrow bool(ref const(SysTime) rhs) both match argument list for opEquals
std\file.d(2103): Error: function std.datetime.SysTime.opEquals called with argument types:
((const(SysTime)) const)
matches both:
std.datetime.SysTime.opEquals(const(SysTime) rhs)
and:
std.datetime.SysTime.opEquals(ref const(SysTime) rhs)
std\file.d(2103): Error: overloads const pure nothrow bool(const(SysTime) rhs) and const pure nothrow bool(ref const(SysTime) rhs) both match argument list for opEquals
std\file.d(2103): Error: function std.datetime.SysTime.opEquals called with argument types:
((const(SysTime)) const)
matches both:
std.datetime.SysTime.opEquals(const(SysTime) rhs)
and:
std.datetime.SysTime.opEquals(ref const(SysTime) rhs)
std\file.d(2103): Error: overloads const pure nothrow bool(const(SysTime) rhs) and const pure nothrow bool(ref const(SysTime) rhs) both match argument list for opEquals
std\file.d(2103): Error: function std.datetime.SysTime.opEquals called with argument types:
((const(SysTime)) const)
matches both:
std.datetime.SysTime.opEquals(const(SysTime) rhs)
and:
std.datetime.SysTime.opEquals(ref const(SysTime) rhs)

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2012

@WalterBright, please read the summary. This pull is an additional fix of dmd/pull/41.
Before ref/non-ref overload issue (it will be fixed by dmd/pull/41), we MUST not merge only this pull.
@Andarex and I argue that you should merge dmd/pull/41 first to merge this pull.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2012

Please also fix opEquals() to be @safe or @trusted.

BigInt, SList, and Array are struct, not class. Adding such attribute is better, but it's not related to the purpose of this pull.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2012

@WalterBright, Can you reopen this pull? If it isn't possible, I'll post a new pull exactly the same as this.

@WalterBright
Copy link
Member

I did pull 41 first. It does not compile phobos.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2012

Ouch! Sorry, dmd/pull/41 was silently broken by others pull. And sorry my irrelevant comment (I didn't noticed you already merged the two pulls).
The problem is here. But I cant post a fixup pull while a few hours.

@WalterBright
Copy link
Member

reverted for the moment, as well as pull 41.

@yebblies
Copy link
Member

@WalterBright Could you please push the reverts then?

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2012

I've posted a revert pull that contains a small fixup. Please merge #753.

9rnsr pushed a commit to 9rnsr/phobos that referenced this pull request Feb 21, 2012
9rnsr added a commit to 9rnsr/phobos that referenced this pull request Feb 21, 2012
WalterBright added a commit that referenced this pull request Feb 21, 2012
Revert "Revert "Merge pull request #120 from 9rnsr/rvalue-struct-literal""
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.

5 participants