Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Issue 19924 - Make core.bitop.bswap(ulong) work in betterC #2621

Merged
merged 2 commits into from
Mar 12, 2020

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented May 31, 2019

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented May 31, 2019

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
19924 enhancement Make core.bitop.bswap(ulong) work in betterC

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 + druntime#2621"

auto sv = Split64(v);
version (CoreDdoc) {}
else version (LDC) { /+ LDC treats bswap(ulong) as an intrinsic so don't change the mangling. +/ }
else version (GNU) { /+ GDC treats bswap(ulong) as an intrinsic so don't change the mangling. +/ }
Copy link
Member Author

Choose a reason for hiding this comment

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

Does it make sense to special-case these since LDC and GDC maintain their own versions of druntime anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. You could also just use pragma(inline, true) to make it work for betterC instead of dummy-templatizing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

That works with dmd -betterC -inline but not if -inline is omitted (I just tested).

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a DMD bug then, https://dlang.org/spec/pragma.html#inline:

The default inline behavior is typically selectable with a compiler switch such as -inline.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's factually accurate. GDC maintains its own implementation of rt, everything else is ad verbatim.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to special-case these since LDC and GDC maintain their own versions of druntime anyway?

Even if they maintain their own fork of druntime I think they prefer to have as few differences as possible.

Copy link
Member

Choose a reason for hiding this comment

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

As far as I'm aware, the regression introduced in #2426 has never been fixed either (GDC treats all of core.checkedint as intrinsics, so mangling was broken there too).

Copy link
Member Author

Choose a reason for hiding this comment

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

@n8sh n8sh force-pushed the issue-19924 branch 2 times, most recently from 80935a9 to b1291c3 Compare June 7, 2019 15:34
@dlang-bot dlang-bot added the Enhancement New functionality label Jun 24, 2019
@thewilsonator thewilsonator requested a review from Geod24 as a code owner March 9, 2020 04:09
@dlang-bot dlang-bot merged commit 322f2ea into dlang:master Mar 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants