Proposed templated opEquals #1087
Conversation
marler8997
force-pushed
the
opEquals
branch
3 times, most recently
from
157b0ee to
adf0786
Compare
|
Without committing any opinion whether or not this is appropriate, you also need to duplicate all the code to object.di. |
Right, because the compiler need to see the code to instantiate a template. |
|
Thanks @schveiguy , I forgot about |
| @@ -253,6 +259,16 @@ class TypeInfo | |||
| auto ti = cast(const TypeInfo)o; | |||
| return ti && this.toString() == ti.toString(); | |||
| } | |||
| bool opEquals(const TypeInfo ti) | |||
There was a problem hiding this comment.
I thought about final but since the generic opEquals wasn't final I just followed suite. I guess it really depends on if anyone would want to override the opEquals method. What do you think?
There was a problem hiding this comment.
TypeInfo's derivatives are all defined in druntime. If none of them define opEquals, then I think it's safe to make this final. In any case, it's an implementation detail that can be changed later.
marler8997
force-pushed
the
opEquals
branch
2 times, most recently
from
bcd73ed to
959e106
Compare
|
I'm not sure I quite understand why this is failing. From digging a little bit, it looks like templates inside druntime are not visible to code outside druntime...is this correct? |
|
You are messing with a very fundamental piece of the runtime. Errors get very very weird here. In answer to your question -- templates inside druntime are available to outside code, as long as the template is visible. Otherwise, all the templates in object.di would never work! Note, the compiler can do weird things assuming certain things about druntime. What those are, and whether they are causing this error, I'm not sure. |
I'm actually quite sure that's the issue. I think the compiler calls You may need a dmd pull to get this to work... |
|
I think I've figured some things out. It looks like templates inside object_.d are not visible but templates inside object.di are (even when I put the signature of the template inside object.di). After making the changes...there are some errors in phobos...is there a way to tie two PR's together for druntime and phobos? |
Not currently. why not try building locally and fix the issues? Perhaps it's something that would benefit phobos all by itself. |
|
I've made the changes in phobos to fix the build. I don't know why these changes were necessary but for some reason they were. I've made a PR here: dlang/phobos#2848 |
|
Is there a way to mark this PR as dependent on dlang/phobos#2848 ? |
|
For some reason, when I remove the generic opEquals(Object,Object) function, it causes structs that do not have an opEquals method to not be able to be used as keys for associative arrays. I get the error: I get this error for |
| } | ||
|
|
||
| bool opEquals(Object lhs, Object rhs) | ||
| bool opEquals(T,U)(T lhs, U rhs) |
There was a problem hiding this comment.
Maybe it would be better to restrict T and U to class types. You wouldn't need all the static ifs anymore.
Using a template specialization:
bool opEquals(T : const Object, U : const Object)(T lhs, U rhs)
|
Something I think you haven't considered: Currently, a non-const opEquals is called even on const/immutable objects. That's what the "A hack for the moment." overload did. As far as I can see you're breaking that here. I don't know how important it is to keep that problematic behaviour alive, but it's definitely something to be considered. |
|
There's been a pull request by @jmdavis for the same thing: #459. It's been blocked by issue 12537. |
|
@aG0aep6G Thanks for referencing that. I looked for a PR for opEquals but didn't find one. |
|
dmd tests still fail, this PR should be closed since it is a duplicate of #459 . |
Templated opEquals.