don't use memcmp() for struct equality, do memberwise comparisons per the spec by WalterBright · Pull Request #9331 · dlang/dmd

WalterBright · 2019-02-07T10:58:08Z

I.e. if the structs fit in 2 or fewer registers use a member-wise compare.

dlang-bot · 2019-02-07T10:58:10Z

Thanks for your pull request, @WalterBright!

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 + dmd#9331"

WalterBright · 2019-02-07T11:09:57Z

Hmm. Needs more work.

WalterBright · 2019-02-07T23:30:02Z

Who knows why these tests are failing. Can't tell anything from the logs. For one thing, the logs should STOP when there's a failure, not continue going on for thousands of lines. The thing that failed should be the last line. The tests in test/runnable are all simple to diagnose with simple logs.

Geod24 · 2019-02-08T00:45:20Z

the logs should STOP when there's a failure, not continue going on for thousands of lines

True, but not trivial to do, especially if we want to run tests concurrently.

Who knows why these tests are failing.

There seems to be multiple reason. One I was able to dig out pretty easily on Windows is:

std\variant.d(270): Warning: statement is not reachable

Handy link to L270

WalterBright · 2019-02-08T04:12:51Z

Can't repro the variant.d issue, either on Linux or Windows. Sigh. Looking at the code in variant.d, I don't see any reason for the warning, either. Can anyone else repro a problem? It's only a one line change to the compiler.

WalterBright · 2019-02-08T04:34:18Z

@schveiguy @wilzbach as recent committers to std.variant, can you perhaps help out with what is going on here?

The one line change to the compiler replaces a bit compare of struct equality with a member-wise compare for some structs.

WalterBright · 2019-02-08T06:40:11Z

Spec reference: dlang/dlang.org#2572

WalterBright · 2019-02-08T06:57:34Z

Revised the PR so it is in conformance with the spec.

WalterBright · 2019-02-08T07:42:04Z

I think this will need a -preview=fieldcmp switch, because it is apparently breaking existing code. But that is blocked by #9206

WalterBright · 2019-02-08T08:27:42Z

Rather than wait for #9206, I just went ahead and added a -preview=fieldwise switch directly and unblocked this.

changelog/no-cmpsb.dd

aG0aep6G · 2019-02-08T10:54:43Z

Related issue: 16216.

changelog/no-cmpsb.dd

WalterBright · 2019-02-08T21:54:02Z

@aG0aep6G Thank you, I wasn't aware of that bug report. This should fix it (but not for is comparisons).

…ns per the spec

WalterBright · 2019-02-09T00:21:07Z

@wilzbach @thewilsonator ready to pull now.

wilzbach

LGTM.
Though I want to point out that we shouldn't abuse the new -preview too much as we still aren't planning to do a D3 (or D 2020 (?)) in the foreseeable future (?) and thus breaking changes must still go through a deprecation phase in which the users can still compile their code, but are warned about the change.
Just by adding a -preview switch a user won't be warned that this code is affected by this breaking change.

wilzbach · 2019-02-09T01:21:18Z

changelog/no-cmpsb.dd

+The spec requires that the comparison be done in a memberwise manner.
+This brings the implementation in line with this.
+
+$(LINK2 https://dlang.org/spec/expression.html#equality_expressions, Equality Expressions)


This links stands a bit out in the flow of the reader:

Maybe directly link it as e,g, "Equality Expressions can break ..."?

wilzbach · 2019-02-09T01:47:31Z

True, but not trivial to do, especially if we want to run tests concurrently.

Isn't this more a "feature" that someone actively worked into the testsuite?
I think the idea was that it takes quite a long time to get feedback from all machines, so you at least want to get a full list of all failed tests.
Though I absolutely agree that all failed tests should be printed as a summary at the end.
If we agree to switch over to test/run.d on the auto-tester this could be added easily...

@schveiguy @wilzbach as recent committers to std.variant, can you perhaps help out with what is going on here?
The one line change to the compiler replaces a bit compare of struct equality with a member-wise compare for some structs.

Git blame points to five years ago:
https://github.com/dlang/phobos/blame/e10d6a7e973ed85ccceee21088fa716433429583/std/variant.d#L270

WalterBright · 2019-02-09T02:17:28Z

The problem is I don't know how to detect that the user code would break with this. The user code that relies on the old behavior is wrong in any case. Also, the -revert switch will save those who suspect the new behavior is causing them problems.

jacob-carlborg · 2019-02-09T09:32:21Z

Just by adding a -preview switch a user won't be warned that this code is affected by this breaking change.

Before this will be turned on by default it needs to go through a deprecation period.

jacob-carlborg · 2019-02-09T09:34:03Z

If we agree to switch over to test/run.d on the auto-tester this could be added easily...

My unit test runner already does this 😃: #9333.

Introduced in PR dlang#9331, this was changed from a normal bug fix to a -preview switch because enabling it triggered a suprious "statement is not reachable" warning in Phobos. That warning has since been removed (in PR dlang#15568).

Introduced in PR #9331, this was changed from a normal bug fix to a -preview switch because enabling it triggered a suprious "statement is not reachable" warning in Phobos. That warning has since been removed (in PR #15568).

thewilsonator approved these changes Feb 7, 2019

View reviewed changes

WalterBright added the Review:WIP Work In Progress - not ready for review or pulling label Feb 7, 2019

WalterBright force-pushed the no-cmpsb branch 2 times, most recently from 6d73c26 to 17dd56f Compare February 7, 2019 22:53

WalterBright force-pushed the no-cmpsb branch from 17dd56f to d9cca9a Compare February 8, 2019 06:57

WalterBright changed the title ~~don't use memcmp() for small struct equality~~ don't use memcmp() for struct equality, do memberwise comparisons per the spec Feb 8, 2019

WalterBright force-pushed the no-cmpsb branch from d9cca9a to facec04 Compare February 8, 2019 07:14

WalterBright added Merge:Blocked and removed Review:WIP Work In Progress - not ready for review or pulling labels Feb 8, 2019

WalterBright force-pushed the no-cmpsb branch from facec04 to 32dfdc3 Compare February 8, 2019 07:45

WalterBright mentioned this pull request Feb 8, 2019

Refactor -transition and -dip to -transition, -revert and -preview #9206

Merged

WalterBright force-pushed the no-cmpsb branch from 32dfdc3 to f1cfb1e Compare February 8, 2019 08:26

WalterBright requested review from Geod24, MartinNowak, ibuclaw and rainers as code owners February 8, 2019 08:26

WalterBright removed the Merge:Blocked label Feb 8, 2019

WalterBright force-pushed the no-cmpsb branch from f1cfb1e to bfdfd3e Compare February 8, 2019 08:47

jacob-carlborg reviewed Feb 8, 2019

View reviewed changes

changelog/no-cmpsb.dd Outdated Show resolved Hide resolved

wilzbach reviewed Feb 8, 2019

View reviewed changes

changelog/no-cmpsb.dd Outdated Show resolved Hide resolved

WalterBright force-pushed the no-cmpsb branch from bfdfd3e to 26200d6 Compare February 8, 2019 21:30

don't use memcmp() for small struct equality, do memberwise compariso…

4accb77

…ns per the spec

WalterBright force-pushed the no-cmpsb branch from 26200d6 to 4accb77 Compare February 8, 2019 22:30

wilzbach approved these changes Feb 9, 2019

View reviewed changes

WalterBright merged commit 3fa1978 into dlang:master Feb 9, 2019

WalterBright deleted the no-cmpsb branch February 9, 2019 02:06

pbackus mentioned this pull request Dec 8, 2024

Enable -preview=fieldwise by default #17495

Merged

Uh oh!

Conversation

WalterBright commented Feb 7, 2019

Uh oh!

dlang-bot commented Feb 7, 2019

Bugzilla references

Testing this PR locally

Uh oh!

WalterBright commented Feb 7, 2019

Uh oh!

WalterBright commented Feb 7, 2019

Uh oh!

Geod24 commented Feb 8, 2019

Uh oh!

WalterBright commented Feb 8, 2019

Uh oh!

WalterBright commented Feb 8, 2019

Uh oh!

WalterBright commented Feb 8, 2019

Uh oh!

WalterBright commented Feb 8, 2019

Uh oh!

WalterBright commented Feb 8, 2019

Uh oh!

WalterBright commented Feb 8, 2019

Uh oh!

Uh oh!

aG0aep6G commented Feb 8, 2019

Uh oh!

Uh oh!

WalterBright commented Feb 8, 2019

Uh oh!

WalterBright commented Feb 9, 2019

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach Feb 9, 2019

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Feb 9, 2019

Uh oh!

WalterBright commented Feb 9, 2019

Uh oh!

jacob-carlborg commented Feb 9, 2019

Uh oh!

jacob-carlborg commented Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jacob-carlborg commented Feb 9, 2019 •

edited

Loading