Partial fix for issue# 9769. #459

jmdavis · 2013-03-21T07:42:12Z

This makes it so that the global opEquals function is templated. It is
therefore now possible for classes to implement opEquals without
overiding Object's opEquals method, including giving it different
attributes (such as const).

However, the hack version of the global opEquals which takes const
Objects and casts away const has been left for the moment, and the
opEquals on Object has not been touched. At minimum, before the hack
version can go away, work will need to be done on the other classes in
object_.d to make them handle const properly. Also, the opEquals on
Object will probably have to go away before that if we don't want to
break code, as it's the only reason that const Objects have been able to
be compared before now, and until types stop using Object's opEquals,
that's still the only way that they'll be able to be compared. But at
least with these changes, it'll be possible for classes to be compared
without it if they change their signatures for opEquals to not take
Object.

http://d.puremagic.com/issues/show_bug.cgi?id=9769

MartinNowak · 2013-03-21T08:27:23Z

src/object.di

-bool opEquals(Object lhs, Object rhs);
+bool opEquals(T, S)(T lhs, S rhs)
+    if (is(T == class) && is(S == class) &&
+       is(typeof({bool a = lhs.opEquals(rhs); bool b = rhs.opEquals(lhs);})))


How about is(typeof(lhs.opEqual(rhs)) : bool) && is(typeof(rhs.opEquals(lhs)) : bool)?

opEquals should never return anything other than bool on any type.

But I probably should change it so that it specifically checks that the type is exactly bool rather than that it assigns to bool.

MartinNowak · 2013-03-21T08:55:42Z

Hi Kenji (@9rnsr), can you have a look at this too?

MartinNowak · 2013-03-21T09:14:05Z

Can we get a DIP for the whole topic?

jmdavis · 2013-03-21T09:29:01Z

@dawgfoto Okay. I fixed the template constraint and added the missing test (good catch, by the way).

Can we get a DIP for the whole topic?

Certainly, but that will take some time as it really should include information on how the transition will be done, and that will take some time to sort out. This small piece with opEquals is straightforward enough that it doesn't require planning much, but a DIP should include info on opEquals, opCmp, toHash, and toString (not just opEquals), and some of the ramifications get somewhat complicated (e.g. toHash and toString probably won't be able to have a clean deprecation process due to override being required if Object has them and override being illegal if Object doesn't have them).

But we do need to get the ball rolling on figuring out what changes need to be made for all of this and start getting them made, or it won't be happening for a while, and that much more code will end up breaking as a result.

9rnsr · 2013-03-21T11:07:51Z

One question:
Which do you intend:

Object is still the root of all of classes.
"user-defined root class" would be allowed.
?

jmdavis · 2013-03-21T11:17:00Z

One question:
Which do you intend:

Object is still the root of all of classes.

"user-defined root class" would be allowed.
?

#1. I don't think that it was really suggested (and certainly wasn't decided on) that Object would be gotten rid of. It would still be there with the monitor and whatever else we think should be on it. It still makes it so that you can have collections of Object if you really want to and return it from Object.factory and whatnot. It's just that it won't have opEquals, opCmp, toHash, or toString on it, because they don't need to be on there, and having them on there has turned out to be too restrictive with regards to their signatures. Maybe we wouldn't have Object if we were to go back to the beginning and redesign everything, but removing it at this point would be too big a change IMHO, and it still has some value even if it doesn't have much on it. Removing those 4 functions is disruptive enough, but we need to in order to resolve issues with const, pure, @safe, and nothrow.

Any class hierarchy would have a base class that was used in their opEquals or opCmp functions (like how Object is used with them now), but the attributes on them would then be decided by the programmer who is designing that hierarchy. Object would still be under it all though.

9rnsr · 2013-03-21T11:29:40Z

@jmdavis OK, I understand. Thanks for quick answer.

9rnsr · 2013-03-21T11:34:22Z

src/object.di

+bool opEquals(T, S)(T lhs, S rhs)
+    if (is(T == class) && is(S == class) &&
+        is(typeof(lhs.opEquals(rhs)) == bool) &&
+        is(typeof(rhs.opEquals(lhs)) == bool))


For structs, they have already less restriction for the return type of opEquals than this.
I think this should be is(typeof(lhs.opEquals(rhs)) : bool) && is(typeof(rhs.opEquals(lhs)) : bool)).

Then I would strongly argue that structs should be more restrictive rather than making this less restrictive. TDPL specifically says that opEquals returns bool, and I don't think that there's a valid reason for it to ever return anything else. We already got rid of equals_t, which existed solely because D1 had made the mistake of originally defining opEquals as returning int. opEquals should always return bool.

I'd say it's okay to simplify here and just require bool.

I also agree. Also, because of the way auto and template inferal works, "implicitly convertible to" just isn't that useful:

auto b = (a == b); //Is b a bool, who knows?

void foo(T)(T t); foo(a == b); //Calls foo!bool ? who knows?

Could this maybe be used for some expression template trickery?

Could this maybe be used for some expression template trickery?

I'm afraid that I don't know what you mean. I'm not familiar with expression templates.

@9rnsr: okay with just requiring bool?

Ok. I can agree.

andralex · 2013-03-24T13:14:32Z

I think it's damn straight intolerable that we need to duplicate swaths of real code between object.di and object_.d. Any ideas on how we can fix that? I'm thinking of something along the lines of mixin(import). Speaking of the larger issue, I'm willing to give this a pass as a special base runtime thing; if we figure this is a pattern that regular projects need to follow we should probably change the language.

MartinNowak · 2013-03-24T17:44:50Z

if we figure this is a pattern that regular projects need to follow we should probably change the language

D needs better support for hiding implementation details.

jmdavis · 2013-03-25T01:26:32Z

Okay. Updated per the comments, and I also removed the overloads for opEquals in the tests which used Object. I'd had them there so that I could ensure that the tests passed with the old code before templatizing the global opEquals, but I forgot to remove them after they weren't necessary anymore.

DmitryOlshansky · 2014-02-26T18:57:41Z

Seems like long gone day. Any fuel in this fire? Okay to close?

andralex · 2014-03-16T04:45:52Z

This looks desirable. @jmdavis? I'll close, please reopen if You Are The Chosen One.

jmdavis · 2014-04-03T06:05:15Z

I think that this is very desirable. It's the first step in fixing the problem with opEquals, opCmp, toHash, and toString being on Object. The real problems though are that there will need to be compiler changes and probably changes in the AA implementation to finish the job. This is the easy part.

I should have pushed for it be merged previously, but I've been very busy, and it fell by the wayside. AFAIK though, it's just as valid now as when I first opened it.

jmdavis · 2014-04-03T06:06:55Z

I just rebased with the latest master, since it's old, and there were no merge conflicts.

jmdavis · 2014-04-03T06:12:51Z

Okay. I'll have to close this temporarily. I think that it has an infinite loop or something in it, since when I run the unit tests, they never finish. I'll have to investigate what's wrong, since there was no problem like this before.

jmdavis · 2014-04-03T07:27:31Z

Yeah, it looks like some screwy stuff is going on with the overload rules right now that results in the fully templatized overload never getting called:

https://d.puremagic.com/issues/show_bug.cgi?id=12510

MartinNowak · 2014-04-03T08:18:34Z

I commented on the bug report. Also I'm currently working on a new AA implementation, so this comes in time.

jmdavis · 2014-04-05T11:36:58Z

Okay, I think that I have a function signature that works with the new overload rules. Certainly, the unit tests are now passing, whereas they were resulting in infinite recursion before.

Regardless, given how we're dealing with const today and how I'm forced to continue to cast it away in these changes to opEquals, we're going to have an interesting time transitioning to a situation where an opEquals that isn't const doesn't work with const objects without breaking a lot of code that uses const with classes.

This makes it so that the global opEquals function is templated. It is therefore now possible for classes to implement opEquals without overiding Object's opEquals method, including giving it different attributes (such as const). However, the hack version of the global opEquals which takes const Objects and casts away const has been left for the moment, and the opEquals on Object has not been touched. At minimum, before the hack version can go away, work will need to be done on the other classes in object_.d to make them handle const properly. Also, the opEquals on Object will probably have to go away before that if we don't want to break code, as it's the only reason that const Objects have been able to be compared before now, and until types stop using Object's opEquals, that's still the only way that they'll be able to be compared. But at least with these changes, it'll be possible for classes to be compared without it if they change their signatures for opEquals to not take Object.

jmdavis · 2014-04-05T13:02:36Z

Drat. It looks like the dmd tests are failing even though the druntime tests and phobos tests are fine. I didn't think to run the dmd tests. I'll have to investigate what broke.

jmdavis · 2014-04-07T11:44:32Z

The dmd tests were failing due to the fact that is(T == class) fails if T is a typedef (which makes me all the happier that typedef has been deprecated, since that's hideous and would wreak havoc in generic code). I have a fix for that, but unfortunately, simply templatizing opEquals triggers a compiler bug in a dmd test that uses alias:

https://d.puremagic.com/issues/show_bug.cgi?id=12537

And all it takes to trigger that is to templatize opEquals by adding () - no other changes are needed. So, until that's fixed, I can't actually get my changes to work.

marler8997 · 2015-01-07T00:31:22Z

src/object_.d

    return opEquals(cast()lhs, cast()rhs);
 }

-bool opEquals(Object lhs, Object rhs)
+private void _testOpEquals(T, U)(bool expected, T lhs, U rhs, size_t line = __LINE__)


Would it be better to move _testOpEquals and _testOpEquals2 inside the unittest block? That way it will not be included in a non-unittest build. (Note: if you do this then the private modifier will need to be removed)

IIRC, templatizing functions inside a unittest block doesn't work, or if it does, you only get to instantiate it once. But the function can be versioned with version(unittest) easily enough.

Regardless, I need to rebase this and try the workaround proposed in https://issues.dlang.org/show_bug.cgi?id=12537 to see if I can get this working. IIRC, it was working perfectly fine before except for the win32 build thanks to bug# 12537. But without that bug being fixed or having a workaround, it was pretty much dead in the water.

Maybe templates inside unittest blocks has been fixed because I've already rebased this commit and made that change and it seems to work for me (https://github.com/D-Programming-Language/druntime/pull/1087/files#diff-7eac7eb46e31907f148813e793155274R197). The DMD tests are still failing, but I could try to find a workaround as well. I'll spend some time on it today and see what I can figure out. I'll let you know if I make any progress.

It looks like there's some sort of issue when alias this comes into play. It's probably a compiler bug, but it'll need to be isolated so that we can be sure and report it appropriately - though if it is a compiler bug, it's likely another blocker. :(

mleise · 2016-09-21T18:10:52Z

I have an AA implementation and promised myself to be disciplined and with with const and pure nothrow @nogc hashing and equality functions. I also added @safe to see how far I get with that. For the first time today I made an object an AA key and DMD didn't like any of the above attributes. Ok, I thought, maybe pure nothrow @nogc was a bit much to ask for, but non-@safety seemed a bit of a harsh requirement for an often needed data structure.

So where are you guys with this proposal now? Is alias this still problematic?

jmdavis · 2016-09-21T22:40:17Z

@mleise The current attempt at templatizing opEquals is here: #1439

But it's gotten almost no attention, so it hasn't been merged. Now, that still leaves plenty of other work that needs to be done towards being able to actually remove opEquals from Object, but if the free function, opEquals is templatized, you should be able to define your own opEquals on your classes that has the attributes that you want - at least as long as you make it accept a base class other than Object so that it's not actually overriding the opEquals in Object.

andralex · 2016-09-21T23:21:06Z

@jmdavis I encourage you to open threads on the forum about neglected PRs so as to bring attention to them. Thanks!

mleise · 2016-09-22T20:01:18Z

@jmdavis Thanks for the update.

MartinNowak reviewed Mar 21, 2013
View reviewed changes

9rnsr reviewed Mar 21, 2013
View reviewed changes

andralex closed this Mar 16, 2014

jmdavis reopened this Apr 3, 2014

jmdavis closed this Apr 3, 2014

jmdavis reopened this Apr 5, 2014

jmdavis closed this Apr 5, 2014

aG0aep6G mentioned this pull request Jan 6, 2015

Proposed templated opEquals #1087

Closed

marler8997 reviewed Jan 7, 2015
View reviewed changes

jmdavis deleted the opEquals branch September 21, 2016 22:41

Uh oh!

Partial fix for issue# 9769. #459

Partial fix for issue# 9769. #459

Uh oh!

Conversation

jmdavis commented Mar 21, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak commented Mar 21, 2013

Uh oh!

MartinNowak commented Mar 21, 2013

Uh oh!

jmdavis commented Mar 21, 2013

Uh oh!

9rnsr commented Mar 21, 2013

Uh oh!

jmdavis commented Mar 21, 2013

Uh oh!

9rnsr commented Mar 21, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Mar 24, 2013

Uh oh!

MartinNowak commented Mar 24, 2013

Uh oh!

jmdavis commented Mar 25, 2013

Uh oh!

DmitryOlshansky commented Feb 26, 2014

Uh oh!

andralex commented Mar 16, 2014

Uh oh!

jmdavis commented Apr 3, 2014

Uh oh!

jmdavis commented Apr 3, 2014

Uh oh!

jmdavis commented Apr 3, 2014

Uh oh!

jmdavis commented Apr 3, 2014

Uh oh!

MartinNowak commented Apr 3, 2014

Uh oh!

jmdavis commented Apr 5, 2014

Uh oh!

jmdavis commented Apr 5, 2014

Uh oh!

jmdavis commented Apr 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mleise commented Sep 21, 2016

Uh oh!

jmdavis commented Sep 21, 2016

Uh oh!

andralex commented Sep 21, 2016