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

dmd.root.array: Add sort method, wrapping qsort #10896

Merged
merged 3 commits into from
Mar 12, 2020

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Mar 11, 2020

This makes it way more user-friendly to work with.
Unfortunately I cannot use it in the backend because it is using -betterC.
Will be useful when implementing GNU ABI Tags too.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

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

@thewilsonator
Copy link
Contributor

ERROR: dmd/root/array.d(319): Error: function `core.stdc.stdlib.qsort` is not `nothrow`
dmd/root/array.d(314): Error: `nothrow` function `dmd.root.array.Array!(CaseStatement).Array.sort!(sort_compare).sort` may throw
dmd/statementsem.d(2721): Error: template instance `dmd.root.array.Array!(CaseStatement).Array.sort!(sort_compare)` error instantiating

@Geod24 Geod24 force-pushed the array-sort branch 2 times, most recently from 060fb7e to bfca2ce Compare March 11, 2020 12:20
@thewilsonator
Copy link
Contributor

dmd\libmscoff.d(517): Error: template instance `sort!((ppe1, ppe2) => (**ppe1).opCmp(**ppe2))` cannot use local `__lambda3` as parameter to non-global template `sort(alias pred)()

assert(dstrcmp("Baguette", "Croissant") == -1);
assert(dstrcmp("Croissant", "Baguette") == 1);

static assert(dstrcmp("Baguette", "Croissant") == -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just couldn't help yourself? :-)

While not used yet, it is useful as a predicate for 'Array(T).sort',
and will be used by the ABI tags feature.
It has been directly copied from druntime, with added tests.
int a;
}

Array!OtherStruct arr2;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh dscanner complains about unused variable here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm maybe use

static assert (!is(typeof((){Array!OtherStruct arr2; arr2.sort()}();)));
``

We already have find, but no sort.
This can potentially hidden behind a static if for types that don't support it
(e.g. types with elaborate move semantics), but we don't have any in arrays ATM.
@Geod24
Copy link
Member Author

Geod24 commented Mar 12, 2020

Satisfied the style checker.

@dlang-bot dlang-bot merged commit 0a39108 into dlang:master Mar 12, 2020
@Geod24 Geod24 deleted the array-sort branch March 12, 2020 09:12
return this;
}

static if (is(typeof(this.data[0].opCmp(this.data[1])) : int))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would appear that this line triggers a silent forward reference bug when compiling frontend modules separately. The early forced semantic of typeof(this.data[0]) (a ClassDeclaration when compiling mtype.d) messes up the vtable calculation, and the offset is wrong (when calling isBaseOf).

This happens in at least the dmd-cxx implementation, a suitable reduction and testing 2.066 will need to be done as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduced, any compiler older than 2.085.1 runs the risk of producing a corrupted compiler if you are using incremental/one module at a time compilation strategy (2.085.0 is the last broken release).

FYI @kinke, or ping any other LDC devs so you don't repeat the time I spent looking at this issue. I've already committed a fix to dmd-cxx branch, see the cross-referenced merged PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibuclaw :
Could you check if using a template constraint would work around the problem ? And if not, we can move the static assert in the body.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think template constraints would work just fine. The reduced test I added reports the correct offset at least...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, on further testing even if we fix the problematic static if, you cannot actually use this function anyway, as it relies on dual-context pointers, which is neither implemented in gdc nor ldc.

Copy link
Member Author

@Geod24 Geod24 Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where it does ? Let's move the discussion to #10930 though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants