Lower dmd CmpExp between classes to __cmp call

[edi33416]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#2562"

[edi33416]

Temporarily removing tests that call the compiler lowering code, in order to unblock the build process.
This is required as the PRs depend on each other.

[jacob-carlborg]

Just to be aware, this will now include C++ and Objective-C classes.

[jacob-carlborg]

Please add tests for C++ and Objective-C classes or adjust the signature and the DMD PR if this should not be called with C++ and Objective-C classes.

[edi33416]

@JinShil @jacob-carlborg I updated the code accordingly to the feedback

[jacob-carlborg]

What’s the advantage of this signature compared to the old one?

You can use __traits(getLinkage) alternatively.

[JinShil]

I think it allows for a more optimal implementation if C1 or C2 is explicitly null. I also think the function can be evaluated at compile-time if one of the arguments is explicitly null. That's what it looks like to me, but I could be wrong.

[JinShil]

If what I said above is true, it would be nice to add some unittest code to demonstrate that and ensure it remains that way now and into the future.

[jacob-carlborg]

But the check for both class and Object looks redundant to me.

[ibuclaw]

Comparing C++ classes is different to comparing D classes.

[andralex]

@jacob-carlborg we don't want to restrict comparison to constant objects. Per https://run.dlang.io/is/iTtY7T, it seems the simplest comprehensive sig is:

int __cmp(C1, C2)(C1 lhs, C2 rhs)
if (is(C1 == class) || is(C1 == interface) || is(C1 : const typeof(null))
        &&
    is(C2 == class) || is(C2 == interface) || is(C2 : const typeof(null)));

[andralex]

@edi33416 later as we introduce ProtoObject we can change the sig appropriately

[andralex]

Also: __cmp should return auto, not int so as to accommodate classes that define opCmp to return e.g. float.

[jacob-carlborg]

we don't want to restrict comparison to constant objects.

You can pass anything to a const parameter: mutable, immutable and const. But of course opCmp would need to be const. Is that the problem?

[jacob-carlborg]

int __cmp(C1, C2)(C1 lhs, C2 rhs)
if (is(C1 == class) || is(C1 == interface) || is(C1 : const typeof(null))
        &&
    is(C2 == class) || is(C2 == interface) || is(C2 : const typeof(null)));

The above constraint will include C++ and Objective-C classes and interfaces and COM interfaces. Is that intentional? In that case there needs to be tests for all of those.

[andralex]

Please change the sig as discussed.

[jacob-carlborg]

Can this be private and the compiler bypasses the protection? Or is there a use case for calling this directly?

[RazvanN7]

@edi33416 this seems almost finished. Any change on taking it to the finish line?

Adressed