Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stable sort does not work with structs that both define opAssign and disable default-initialization #9878

Open
dlangBugzillaToGithub opened this issue Oct 13, 2024 · 0 comments

Comments

@dlangBugzillaToGithub
Copy link

issues.dlang (@jmdavis) reported this on 2024-10-13T10:23:41Z

Transfered from https://issues.dlang.org/show_bug.cgi?id=24810

Description

The fix for https://issues.dlang.org/show_bug.cgi?id=24773 made it so that this code no longer compiles:

void main()
{
    static struct E
    {
        int value;
        int valid = 42;

        this(int value)
        {
            this.value = value;
        }

        @disable this();

        ~this()
        {
            //assert(valid == 42);
        }
    }

    import std.array : array;
    import std.range : chain, only, repeat;
    auto arr = chain(repeat(E(41), 18),
                     only(E(39)),
                     repeat(E(41), 16),
                     only(E(1)),
                     repeat(E(42), 33),
                     only(E(33)),
                     repeat(E(42), 16),
                     repeat(E(43), 27),
                     only(E(33)),
                     repeat(E(43), 34),
                     only(E(34)),
                     only(E(43)),
                     only(E(63)),
                     repeat(E(44), 42),
                     only(E(27)),
                     repeat(E(44), 11),
                     repeat(E(45), 64),
                     repeat(E(46), 3),
                     only(E(11)),
                     repeat(E(46), 7),
                     only(E(4)),
                     repeat(E(46), 34),
                     only(E(36)),
                     repeat(E(46), 17),
                     repeat(E(47), 36),
                     only(E(39)),
                     repeat(E(47), 26),
                     repeat(E(48), 17),
                     only(E(21)),
                     repeat(E(48), 5),
                     only(E(39)),
                     repeat(E(48), 14),
                     only(E(58)),
                     repeat(E(48), 24),
                     repeat(E(49), 13),
                     only(E(40)),
                     repeat(E(49), 38),
                     only(E(18)),
                     repeat(E(49), 11),
                     repeat(E(50), 6)).array();

    import std.algorithm.mutation : SwapStrategy;
    import std.algorithm.sorting : sort;
    arr.sort!((a, b) => a.value < b.value, SwapStrategy.stable)();
}

However, prior to that fix, if you uncommented the comment in the destructor, the assertion failed, because (as the fix was intended to fix), stable sort was assigning to uninitialized memory even when the type defined opAssign. So, while the change caused code to no longer compile, the code that stopped compiling was actually broken.

The fix that caused this code to be a compilation error instead of being buggy code that compiled took the easy route of just using initialized memory, which is fine in the vast majority of cases. However, larger changes will be necessary to deal with types that don't allow default initialization.

The example given here is an altered version of the one from https://issues.dlang.org/show_bug.cgi?id=24809 (which shows a case which the fix for https://issues.dlang.org/show_bug.cgi?id=24773 did not catch and which also needs to be fixed).
@LightBender LightBender removed the P1 label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants