Provide utilities to auto implement opEquals, opCmp and toHash #7049

edi33416 · 2019-05-29T15:55:48Z

This still needs a changelog entry and ddoc'ed style unittests.

dlang-bot · 2019-05-29T15:55:50Z

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 + phobos#7049"

n8sh · 2019-05-31T01:12:08Z

Out of curiosity, what is the vision for how this will relate to DMD clone.d's buildXopEquals/buildXopCmp/buildXtoHash?

PetarKirov · 2019-05-31T07:33:43Z

std/typecons.d

+    }
+    else static if (is (Rhs == struct))
+    {
+        alias ContravariantRhsT = auto ref const Unqual!Rhs;


Unfortunately, I don't think you can use storage classes like auto ref, ref, lazy or scope with aliases.

interesting. If these attributes are accepted but "noop" then their presence should be prevented by the compiler.

Unfortunately, I don't think you can use storage classes like auto ref, ref, lazy or scope with aliases.

It didn't even occur to me that this might be ignored. I agree with @Basile-z that this should throw a compiler error if it's not supported: silently ignoring can lead to hard to discover bugs.

@ZombineDev do you know what is the reason to ignore the storage classes?

Because they are not type constructors, but parameter storage classes and alias can "point" to only types, symbols and occasionally values of which storage classes are neither.
For example, you can't declare a function local or class/struct member variable with the same storage classes as a parameter.

This is unfortunately by design and will take an exceptional DIP to convince W&A otherwise. Manu has tried to argue for making ref a first class citizen (i.e. a type constructor), so it can play nice with std.meta metaprogramming for years in the newsgroup, but has not succeeded so far. Many others have tried to independently argue that scope should be a transitive (again meaning a type constructor like const), but have again failed.

For example, my interpretation of the outcome of this bug report https://issues.dlang.org/show_bug.cgi?id=17764 is that Walter values parameter storage classes not being type constructors more then memory safety.

I wonder what would be @atilaneves take on this.

I don't have an opinion on it yet, but would love to hear arguments for/against/about it.

In general, the problem with parameter storage classes is that they don't compose well. (Other storage classes like static are fine as they are, because you rarely have composition problems there.)

In case of DIP1000 / scope the problem is more dramatic as the compiler simply gives up and allows memory-unsafe code in @safe functions.

Manu has tried to argue for making ref a first class citizen

about ref, as you should know, it has been partially addressed lately (ref for alias function is handled but still not as a type ctor, rather as a special case).

ref for alias function is handled but still not as a type ctor, rather as a special case

@Basile-z I didn't know this, thanks for point this out! Can you point me to the PR that changed this lately?

https://dlang.org/changelog/2.086.0.html#funcliteralsbyref

I know this enhancement, but it's unrelated. The core issue is the following:

alias RI = ref int; pragma (msg, RI); // prints `int` during compilation void inc(RI ri) { ri++; } void main() { int x = 10; x.writeln; // prints `10` at run-time x.inc; x.writeln; // prints `10` at run-time }

That doesn't work either:

alias Ref(T) = ref T; void inc(Ref!int ri) { ri++; } pragma (msg, typeof(&inc)); // prints `void function(int ri)` at compile-time

edi33416 · 2019-06-01T17:25:19Z

Out of curiosity, what is the vision for how this will relate to DMD clone.d's buildXopEquals/buildXopCmp/buildXtoHash?

I might be wrong, but this will not cause trouble for buildXopEquals/buildXopCmp/buildXtoHash.
The mixins in this PR provide the user with greater flexibility, so he can pick and choose what are his significant (to be taken into account) member fields.

The user can even provide the name of a method/property to be used inside the operators. The reason for this is: the user used to have a field X that he now changed to a @property -> his code will continue to work.

Another benefit, imho, is that the implementation is in the library and it's easier for a contributor to understand.

What do you think?

n8sh · 2019-06-01T21:57:47Z

What do you think?

I think it could be convenient. Syntax-wise it might be worth considering an approach based around annotating specific fields (either whitelist or blacklist) like in Java's Project Lombok:

import lombok.EqualsAndHashCode;

@EqualsAndHashCode
public class EqualsAndHashCodeExample {
  private transient int transientVar = 10;
  private String name;
  private double score;
  @EqualsAndHashCode.Exclude private Shape shape = new Square(5, 10);
  private String[] tags;
  @EqualsAndHashCode.Exclude private int id;
  
  public String getName() {
    return this.name;
  }
  
  @EqualsAndHashCode(callSuper=true)
  public static class Square extends Shape {
    private final int width, height;
    
    public Square(int width, int height) {
      this.width = width;
      this.height = height;
    }
  }
}

edi33416 · 2019-06-04T13:04:08Z

What do you think?

I think it could be convenient. Syntax-wise it might be worth considering an approach based around annotating specific fields (either whitelist or blacklist) like in Java's Project Lombok:

import lombok.EqualsAndHashCode;

@EqualsAndHashCode
public class EqualsAndHashCodeExample {
  private transient int transientVar = 10;
  private String name;
  private double score;
  @EqualsAndHashCode.Exclude private Shape shape = new Square(5, 10);
  private String[] tags;
  @EqualsAndHashCode.Exclude private int id;
  
  public String getName() {
    return this.name;
  }
  
  @EqualsAndHashCode(callSuper=true)
  public static class Square extends Shape {
    private final int width, height;
    
    public Square(int width, int height) {
      this.width = width;
      this.height = height;
    }
  }
}

At DConf, @atilaneves also asked me if I have thought about using UDAs to exclude fields.
At the time, I had not. I've given it some thought since and I think we could have UDAs as an enhancement to the current implementation. I'm afraid we could "clash" with some existing UDAs that users have in their codebases.

The reason I'm saying we can have it as an enhancement is because I think we could see how the users use the current implementation and if they feel that UDAs would increase their productivity / make the code nicer.

As a final note, I'm not opposing to using UDAs those are just my thoughts.

atilaneves · 2019-06-08T15:26:07Z

@edi33416:

I'm afraid we could "clash" with some existing UDAs that users have in their codebases.

This is easily disambiguated by using the fully qualified name when/if necessary.

edi33416 · 2019-06-13T12:09:35Z

@atilaneves is UDAs support required as part of this PR or can it come as a future PR?

atilaneves · 2019-06-14T12:00:54Z

@edi33416 I wouldn't say UDA support is required, no.

RazvanN7 · 2021-04-22T08:34:15Z

@edi33416 What are your plans with this PR?

RazvanN7 · 2021-09-15T10:16:05Z

@edi33416 Is there anything blocking this PR?

edi33416 · 2021-09-15T12:20:27Z

@RazvanN7 I don't think there are any blockers for this PR. I've just rebased with master and pushed the updated branch.
Let's see if the checks pass

RazvanN7 · 2021-09-15T13:40:39Z

std/typecons.d

+    assert(t1.toHash() == t2.toHash());
+}
+
+template GetAllFields(T)


This is a public symbol, therefore it needs to be documented. Same goes for the rest.

RazvanN7 · 2021-09-15T13:42:43Z

Since this PR is adding so many public symbols, it requires a changelog entry.

RazvanN7 · 2021-09-28T15:29:27Z

This assert seems to be tripping:

core.exception.AssertError@std/typecons.d(5802): unittest failure

edi33416 · 2021-10-21T10:47:03Z

I've been wondering lately if this should go in std/experimental/typecons.d until we are sure that the mixin interface is solid.
This will prove to be the most useful for the ProtoObject changes, so it does seem counter-intuitive that a (recommended?) helper is experimental, but this would allow us to fix interface issues, if they arise.

What do you think?

RazvanN7 · 2021-10-22T09:50:34Z

@atilaneves what's your stance on this PR?

If you need some background: these methods will come in handy once users start using ProtoObject.

atilaneves

Why one PR for all three functions and not one for each?

atilaneves · 2021-10-25T11:11:46Z

std/typecons.d

@@ -5574,6 +5574,779 @@ private
    extern(C) pure nothrow Object typecons_d_toObject(void* p);
 }

+
+// Implement{Ordered, Equals, Hash} helper function


This seems to be the wrong comment?

std/typecons.d

atilaneves · 2021-10-25T11:13:19Z

std/typecons.d

+            {
+                // TODO: fix this legacy bad behavior, see
+                // https://issues.dlang.org/show_bug.cgi?id=17244
+                static assert(is(U1 == U2), "Internal error.");


Surely there's a better message than "Internal error"?

atilaneves · 2021-10-25T11:14:08Z

std/typecons.d

+        enum len = U.length;
+
+        int r = 0;
+        static foreach (i, unused; U)


static foreach(i; 0 .. len)?

RazvanN7 · 2022-01-07T12:23:39Z

@edi33416 please address @atilaneves 's comments. Also, can you split this up into 3 PRs?

LightBender · 2024-10-27T00:48:12Z

PR closed as stalled. If you wish to resurrect it you are welcome to do so, however it is recommend to split this up in to multiple PR's.

edi33416 requested review from MetaLang and PetarKirov as code owners May 29, 2019 15:55

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label May 29, 2019

edi33416 requested review from andralex and wilzbach May 29, 2019 15:56

PetarKirov reviewed May 31, 2019

View reviewed changes

edi33416 force-pushed the auto_impl_overrides branch from c69b6b3 to a298287 Compare November 14, 2019 15:05

dlang-bot added the Merge:stalled label Jul 21, 2021

edi33416 force-pushed the auto_impl_overrides branch from a298287 to 0789366 Compare September 15, 2021 12:19

dlang-bot removed the Merge:stalled label Sep 15, 2021

RazvanN7 changed the title ~~[WIP] Provide utilities to auto implement opEquals, opCmp and toHash~~ Provide utilities to auto implement opEquals, opCmp and toHash Sep 15, 2021

RazvanN7 reviewed Sep 15, 2021

View reviewed changes

edi33416 added 5 commits October 13, 2021 16:33

WIP add operator overloading utilities

c9b8faf

Add Hash and cleanup code

17089ad

Fix bugs

40389ec

Add ddocs

4068473

Fix deprecation errors

93328de

edi33416 force-pushed the auto_impl_overrides branch from 5135fcb to 93328de Compare October 13, 2021 16:14

edi33416 added 2 commits October 14, 2021 15:35

Fix toHash

9afbfc4

Fix style errors

eb60808

PetarKirov added the Review:Atila Neves label Oct 14, 2021

Fix style errors

7e9fc25

atilaneves requested changes Oct 25, 2021

View reviewed changes

edi33416 mentioned this pull request Oct 29, 2021

Rethinking the default class hierarchy dlang/DIPs#219

Merged

dlang-bot added the Merge:stalled label Apr 7, 2022

LightBender closed this Oct 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide utilities to auto implement opEquals, opCmp and toHash #7049

Provide utilities to auto implement opEquals, opCmp and toHash #7049

edi33416 commented May 29, 2019

dlang-bot commented May 29, 2019 •

edited

Loading

n8sh commented May 31, 2019

PetarKirov May 31, 2019

ghost Jun 1, 2019

edi33416 Jun 1, 2019

PetarKirov Jun 9, 2019

atilaneves Jun 9, 2019

PetarKirov Jun 9, 2019

ghost Jun 9, 2019

PetarKirov Jun 9, 2019 •

edited

Loading

ghost Jun 9, 2019

PetarKirov Jun 9, 2019 •

edited

Loading

edi33416 commented Jun 1, 2019

n8sh commented Jun 1, 2019

edi33416 commented Jun 4, 2019

atilaneves commented Jun 8, 2019

edi33416 commented Jun 13, 2019

atilaneves commented Jun 14, 2019

RazvanN7 commented Apr 22, 2021

RazvanN7 commented Sep 15, 2021

edi33416 commented Sep 15, 2021

RazvanN7 Sep 15, 2021 •

edited

Loading

RazvanN7 commented Sep 15, 2021

RazvanN7 commented Sep 28, 2021

edi33416 commented Oct 21, 2021

RazvanN7 commented Oct 22, 2021

atilaneves left a comment

atilaneves Oct 25, 2021

atilaneves Oct 25, 2021

atilaneves Oct 25, 2021

RazvanN7 commented Jan 7, 2022

LightBender commented Oct 27, 2024

Provide utilities to auto implement opEquals, opCmp and toHash #7049

Provide utilities to auto implement opEquals, opCmp and toHash #7049

Conversation

edi33416 commented May 29, 2019

dlang-bot commented May 29, 2019 • edited Loading

Bugzilla references

Testing this PR locally

n8sh commented May 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov Jun 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov Jun 9, 2019 • edited Loading

Choose a reason for hiding this comment

edi33416 commented Jun 1, 2019

n8sh commented Jun 1, 2019

edi33416 commented Jun 4, 2019

atilaneves commented Jun 8, 2019

edi33416 commented Jun 13, 2019

atilaneves commented Jun 14, 2019

RazvanN7 commented Apr 22, 2021

RazvanN7 commented Sep 15, 2021

edi33416 commented Sep 15, 2021

RazvanN7 Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

RazvanN7 commented Sep 15, 2021

RazvanN7 commented Sep 28, 2021

edi33416 commented Oct 21, 2021

RazvanN7 commented Oct 22, 2021

atilaneves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RazvanN7 commented Jan 7, 2022

LightBender commented Oct 27, 2024

dlang-bot commented May 29, 2019 •

edited

Loading

PetarKirov Jun 9, 2019 •

edited

Loading

PetarKirov Jun 9, 2019 •

edited

Loading

RazvanN7 Sep 15, 2021 •

edited

Loading