Remove __cmp from object.d for aggregates

[JinShil]

Surprisingly it doesn't appear to be utilized anywhere. Let's see what the test suite says.

[dlang-bot]

Thanks for your pull request and interest in making D better, @JinShil! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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 fetch digger
dub run digger -- build "master + druntime#2633"

[JinShil]

Looks like it was introduced here: #1787

@andralex Was this intended to have an accompanying DMD PR, but was never followed through on?

[JinShil]

Please don't merge this PR until @andralex responds. I'm wondering if we need to add a lowering to DMD instead. But it's strange that this overload of __cmp is private.

[jacob-carlborg]

Fine by me if the tests are passing.

[andralex]

The plan here was to have the compiler lower object comparison to this. Also @edi33416 is working on defining __cmp for structs as well (lexicographic in field order).

[JinShil]

Thank you @andralex Closing pending a DMD PR that will utilize this.

[JinShil]

I'm reopening this.

• RTLSYM_OBJ_CMP is no longer used in the compiler
• The old runtime hook _d_obj_cmp no longer exists in druntime. In fact the entire file rt/obj.d no longer exists in druntime.
• It appears that comparing objects in the runtime has been superceded by opCmp

I don't know what the plan is for the code that this PR removes. @andralex, If this is not dead code, could you please elaborate on what needs to be done to utilize it.

[andralex]

Sure, we can always bring this back if needed. The problem with opCmp is that it's a member so if it's final it can't operate on null object. cc @edi33416

[edi33416]

I have this dmd pr and druntime pr to lower calls to __cmp. This was influenced by work at ProtoObject. Should I still keep those or two ProtoObjects shouldn’t be compared and have the user do a dynamic/static cast at call site? I think it would be reasonable to ask the user to do so.

[JinShil]

Thank you @edi33416 It appears the utility of this code has yet to be determined and is pending the adjudication of work on ProtoObject. I'll close this for now until those design decisions are made.

[JinShil]

Should I still keep those or two ProtoObjects shouldn’t be compared and have the user do a dynamic/static cast at call site?

I don't know what's happening as I'm not intimately involved in the design of ProtoObject. I thought, based on this thread, that the value of comparing two objects was in question. I don't know what, if any, decision has been made about that, but I would be interested to know. As I mentioned in that thread, I would prefer a clear distinction between "reference comparisons", "value comparisons", and "hash comparisons".

[andralex]

@edi33416 we need that lowering to compare properly null objects. Otherwise code like this will crash:

class Widget { ... defines opCmp ... }
auto w1 = new Widget;
Widget w2 = null;
if (w2 < w1) {}

If all we do is lower to w1.opCmp(w2) we end up calling a method against a null reference. So please proceed and be bold.