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

Add ability to compare const/immutable classes #17627

Open
dlangBugzillaToGithub opened this issue Nov 19, 2013 · 5 comments
Open

Add ability to compare const/immutable classes #17627

dlangBugzillaToGithub opened this issue Nov 19, 2013 · 5 comments

Comments

@dlangBugzillaToGithub
Copy link

Denis Shelomovskii (@denis-sh) reported this on 2013-11-19T13:08:50Z

Transferred from https://issues.dlang.org/show_bug.cgi?id=11557

Description

Currently because of Issue 1824 `opEquals` and `opCmp` aren't `const`. It's bad but works if these functions are implemented carefully.

Current dmd rejects `opCmp` call:
---
const Object o1, o2;
assert(o1 == o2); // ok, calls `opEquals`
assert(o1 <= o2); // Error: mutable method object.Object.opCmp is not callable using a const object
---

As a result e.g. `std.typecons.Tuple`-s with classes don't have `opCmp` and fails with "TypeInfo.compare is not implemented" `Error` if used as an associative array key. Error at runtime is really nasty.
@dlangBugzillaToGithub
Copy link
Author

verylonglogin.reg commented on 2013-11-19T13:14:09Z

A workaround for `std.typecons.Tuple` regression:
https://github.com/D-Programming-Language/phobos/pull/1707

@dlangBugzillaToGithub
Copy link
Author

k.hara.pg commented on 2013-11-20T01:26:06Z

(In reply to comment #0)
> Currently because of Issue 1824 `opEquals` and `opCmp` aren't `const`. It's bad
> but works if these functions are implemented carefully.
> 
> Current dmd rejects `opCmp` call:
> ---
> const Object o1, o2;
> assert(o1 == o2); // ok, calls `opEquals`
> assert(o1 <= o2); // Error: mutable method object.Object.opCmp is not callable
> using a const object
> ---
> 
> As a result e.g. `std.typecons.Tuple`-s with classes don't have `opCmp` and
> fails with "TypeInfo.compare is not implemented" `Error` if used as an
> associative array key. Error at runtime is really nasty.

Why this issue is marked as "regression"? As far as I know, the code you shown had never worked correctly.

@dlangBugzillaToGithub
Copy link
Author

verylonglogin.reg commented on 2013-11-20T07:31:13Z

(In reply to comment #2)
> Why this issue is marked as "regression"? As far as I know, the code you shown
> had never worked correctly.

Associative arrays use `TypeInfo.compare` and `std.typecons.Tuple` has its `opCmp` with by element comparison for a long time. As `Tuple`-s stopped working as AA keys (it worked at least a year ago) I decided it was a compiler change. If it's not please remove the REGRESSION status.

@dlangBugzillaToGithub
Copy link
Author

k.hara.pg commented on 2013-11-21T07:30:09Z

(In reply to comment #3)
> (In reply to comment #2)
> > Why this issue is marked as "regression"? As far as I know, the code you shown
> > had never worked correctly.
> 
> Associative arrays use `TypeInfo.compare` and `std.typecons.Tuple` has its
> `opCmp` with by element comparison for a long time. As `Tuple`-s stopped
> working as AA keys (it worked at least a year ago) I decided it was a compiler
> change. If it's not please remove the REGRESSION status.

See my comment in your PR. To me the old worked behavior was essentially wrong, and I think it should be kept broken until class const correctness will be fixed.

https://github.com/D-Programming-Language/phobos/pull/1707#issuecomment-28992632

@dlangBugzillaToGithub
Copy link
Author

verylonglogin.reg commented on 2013-11-24T00:56:24Z

Filed Issue 11588 for inconsistency in abilities to compare `const`/`immutable` classes and Issue 11591 for Phobos regression.

This issue is a request to add ability to compare `const`/`immutable` classes just like currently allowed testing for equality. It will fix Issue 11588 if accepted.

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

No branches or pull requests

1 participant