Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

core/atomic.d: Add GDC atomic support code#2831

Merged
dlang-bot merged 1 commit intodlang:masterfrom
ibuclaw:atomicgdc
Jan 1, 2020
Merged

core/atomic.d: Add GDC atomic support code#2831
dlang-bot merged 1 commit intodlang:masterfrom
ibuclaw:atomicgdc

Conversation

@ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Oct 14, 2019

Following on from #2596, rewrite to work with the new atomic API.

This has been peer reviewed by @TurkeyMan

Best viewed with whitespace ignored, as the removals are only highlighted by indenting the DMD specific code into a version(DigitalMars) block.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

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

Copy link
Contributor

@TurkeyMan TurkeyMan left a comment

Choose a reason for hiding this comment

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

I already reviewed this, but here's my 👍

@TurkeyMan
Copy link
Contributor

What's sad is that no unit tests will cover this GDC code... is that fine?

@ibuclaw
Copy link
Member Author

ibuclaw commented Oct 14, 2019

What's sad is that no unit tests will cover this GDC code... is that fine?

Erm, all the unittests in core.atomic.

@TurkeyMan
Copy link
Contributor

Yeah but the DMD CI won't prove this code path, it could regress against GDC?

@ibuclaw
Copy link
Member Author

ibuclaw commented Oct 17, 2019

Yeah but the DMD CI won't prove this code path, it could regress against GDC?

If it isn't syntactically valid, an error would be thrown. Any API changes need to be synchronized, however I would expect the person making the change would do the required due dilligence, otherwise there's no basis for accepting them.

@kinke
Copy link
Contributor

kinke commented Oct 17, 2019

Iain, what's your ultimate goal wrt. these upstreamings? Do you aim for not having to maintain druntime/Phobos forks or do you plan to restrict the upstreamings to manageably-sized diffs?

Edit: Never mind, I've just seen that discussion in the previous PR. Certainly less work for us downstream compilers, but the code doesn't get more readable/tight. Not sure if LDC will follow suit; the recent refactorings are certainly a hassle to adapt to, but on the other hand, upstreaming everything will make refactorings a bigger hassle here upstream, and in the worst case, less likely due to the additional work for potentially interested contributors.

Following on from #2596, rewrite to work with the new atomic API
@dlang-bot dlang-bot merged commit b6aa34f into dlang:master Jan 1, 2020
@ibuclaw ibuclaw deleted the atomicgdc branch January 3, 2020 15:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants