[Issue 13179] AA key type TagIndex now requires equality rather than comparison (page 3)

https://issues.dlang.org/show_bug.cgi?id=13179 --- Comment #20 from Jacob Carlborg <doob@me.com> --- (In reply to Jonathan M Davis from comment #19) > Okay. Clearly, I was not paying enough attention to what was going on here. It always has been the case that opEquals is generated for for structs, if you don't define it. That hasn't changed for 2.066. Rather, in the case where you try and use it as an AA key, it now screams at you for not having defined opEquals, even though the default opEquals was almost certainly fine, because that's what was being used. > > It's a major bug IMHO if lhs.opCmp(rhs) == 0 is not equivalent to lhs == rhs, so if we change the AAs to use opEquals but do not give the error that we're currently giving, then all we'd be doing is exposing an existing bug in someone's code, and that bug probably exists elsewhere in their code, since they probably use opEquals with their keys _somewhere_. > > And it seems kind of messed up to me to require that opEquals be defined just because you're using an AA when the default opEquals already does the right thing. > > IMHO, we should either require that opEquals be defined in general if opCmp is defined (which I think is a bad idea), or we should just use the default opEquals with AAs without complaining about it (much better idea IMHO). The only code that would be negatively affected is already broken anyway. But requiring folks to define opEquals instead of using the perfectly good default just because they're using an AA seems very wrong to me. If you define opCmp it means that the default generated isn't correct for you. If the default opCmp isn't correct, how often will the default opEquals be correct then. I mean, will there be uses where the default opEquals is correct but not the default opCmp. Generating opEquals to call opCmp == 0 if opCmp is defined will be a breaking change but it will only change the behavior if previously lhs.opCmp(rhs) == 0 was _not_ the same as lhs == rhs, which you defined as a bug. So to me this looks like it will fix bugs. --

July 25, 2014

[Issue 13179] AA key type TagIndex now requires equality rather than comparison

Posted by Jonathan M Davis

Permalink

Jonathan M Davis

Permalink

https://issues.dlang.org/show_bug.cgi?id=13179

--- Comment #21 from Jonathan M Davis <jmdavisProg@gmx.com> ---
(In reply to Jacob Carlborg from comment #20)
> If you define opCmp it means that the default generated isn't correct for you. If the default opCmp isn't correct, how often will the default opEquals be correct then. I mean, will there be uses where the default opEquals is correct but not the default opCmp.
> 
> Generating opEquals to call opCmp == 0 if opCmp is defined will be a breaking change but it will only change the behavior if previously lhs.opCmp(rhs) == 0 was _not_ the same as lhs == rhs, which you defined as a bug. So to me this looks like it will fix bugs.

The compiler does not define opCmp for you. If you don't define it, none of the other comparison operators work. You define opCmp, because you want your struct to work with <, <=, >=, and >. == has nothing to do with it. And if you define opCmp so that lhs.opCmp(rhs) == 0 is not equivalent to opEquals, then one of the two needs to be fixed so that they _are_ equivalent; otherwise it's a bug. If that means having to define opEquals instead of the built-in one, then you have to define it, but in many cases, the built-in one is correct, and you simply added opCmp in order to get the other comparison operators.

So, if we make opEquals be generated as lhs.opCmp(rhs) == 0 when opCmp is defined, then that will incur a performance hit, and it will only be more correct if the struct was broken in the first place. So, we'd be penalizing code that was written correctly just to fix bugs in code that didn't define opEquals like when should have. And that still wouldn't fix code where opEquals _was_ defined but it was defined incorrectly.

So, as far as I can see, changing the default opEquals implementation to use opCmp just penalizes the folks who made sure that opCmp was correctly implemented. I don't see any reason to try and fix broken code by changing the language, especially when that penalizes correct code.

And FWIW, Walter agrees with me: http://forum.dlang.org/post/lqsnmu$2f3q$1@digitalmars.com

--

https://issues.dlang.org/show_bug.cgi?id=13179 --- Comment #22 from hsteoh@quickfur.ath.cx --- Alright. I think the forum discussion has convinced me that having opEquals = (opCmp()==0) is a bad idea. Instead, Jonathan's solution is the best: get rid of the compile error that forces you to declare opEquals just because you declared opCmp. Code that defines opCmp that's inconsistent with opEquals is already broken, and we aren't making things worse. Assuming that the user-defined opCmp is consistent with opEquals, then the AA code change that switched from using typeinfo.compare to typeinfo.equals should still work correctly on existing code. So this sounds like the best way to go. --

https://issues.dlang.org/show_bug.cgi?id=13179 Walter Bright <bugzilla@digitalmars.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bugzilla@digitalmars.com --- Comment #23 from Walter Bright <bugzilla@digitalmars.com> --- Anyone know which pull request introduced this regression? --

https://issues.dlang.org/show_bug.cgi?id=13179 --- Comment #24 from hsteoh@quickfur.ath.cx --- Found it with git bisect: it's caused by dmd commit 11a29600c94a599ea670c8ba1a134a6ea5bcf491, which was merged by PR 3731. --

https://issues.dlang.org/show_bug.cgi?id=13179 Walter Bright <bugzilla@digitalmars.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://issues.dlang.org/sh | |ow_bug.cgi?id=13074 --

https://issues.dlang.org/show_bug.cgi?id=13179 --- Comment #28 from github-bugzilla@puremagic.com --- Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/97045eae39d6c10eab020b95f9ee8bcfc01d8fb3 fix Issue 13179 - AA key type TagIndex now requires equality rather than comparison https://github.com/D-Programming-Language/dmd/commit/6363a7f8e20aacb3271d5607c6f024c9b54b21e1 Merge pull request #3813 from WalterBright/revert3731 [reg] fix Issue 13179 - AA key type TagIndex now requires equality rather than... --

Forums