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

Refactor core.atomic : cas into a two-level template#3095

Merged
dlang-bot merged 1 commit intodlang:masterfrom
Geod24:refactor-cas
Aug 4, 2020
Merged

Refactor core.atomic : cas into a two-level template#3095
dlang-bot merged 1 commit intodlang:masterfrom
Geod24:refactor-cas

Conversation

@Geod24
Copy link
Member

@Geod24 Geod24 commented May 4, 2020

This should reduce the amount of boilerplate needed in the implementation,
and hopefully make the code easier to read for everyone.

I want to see how it shows up in the doc though, before we merge this.

@Geod24 Geod24 added the Refactoring No semantic changes label May 4, 2020
@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 + druntime#3095"

@Geod24
Copy link
Member Author

Geod24 commented May 4, 2020

@TurkeyMan : Alright I have to admit when I started refactoring this I missed the point that this was essentially both compare-and-set and compare-and-exchange (as in, ifThis might get written to).

Combining this with templates leads to a very unintuitive API IMO. Things blew up for me when I tried to use compare-and-set with pointers (basically initializing a shared immutable(T)*). Thoughts ?

@TurkeyMan
Copy link
Contributor

TurkeyMan commented May 4, 2020

This is terrifying. Does argument inference work exactly the same with this arrangement?
I have no feel for what the semantics are with this change :/

I don't have time right now to think this through, but I'd like the opportunity to understand this before it's merged.

@Geod24
Copy link
Member Author

Geod24 commented Jun 21, 2020

This is terrifying. Does argument inference work exactly the same with this arrangement?

As long as one does not explicitly specify the type of arguments, and relies on IFTI, everything works the same. This pattern is very helpful when one needs to have some template parameters part of the interface and others not.

I don't have time right now to think this through, but I'd like the opportunity to understand this before it's merged.

Any chance you could give it a look now ?

@Geod24
Copy link
Member Author

Geod24 commented Jun 29, 2020

Ping @TurkeyMan

@n8sh
Copy link
Member

n8sh commented Jul 2, 2020

The change to a two-level template will cause any existing fully-parameterized uses of cas to fail to compile!

@Geod24
Copy link
Member Author

Geod24 commented Jul 20, 2020

The change to a two-level template will cause any existing fully-parameterized uses of cas to fail to compile!

Indeed but fully-parameterized cases of IFTI is not usually something we cater for, unless there's a special-case for cas I'm not aware of ?

@Geod24
Copy link
Member Author

Geod24 commented Jul 20, 2020

Ping @TurkeyMan

@n8sh
Copy link
Member

n8sh commented Jul 24, 2020

I don't see documentation indicating to library users that core.atomic.cas is not supposed to be explicitly parameterized, but as the parameterization was changed a year ago without objection I can't on principle say this is a blocker.

@Geod24 Geod24 requested a review from atilaneves July 25, 2020 21:35
@n8sh n8sh added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Jul 27, 2020
@Geod24
Copy link
Member Author

Geod24 commented Aug 1, 2020

Well it's been more than 72h so adding auto-merge.

@Geod24 Geod24 added auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Aug 1, 2020
This should reduce the amount of boilerplate needed in the implementation,
and hopefully make the code easier to read for everyone.
@Geod24
Copy link
Member Author

Geod24 commented Aug 2, 2020

I don't know what's up with FreeBSD... It doesn't seem spurious, however this was seen in other unrelated PR. Will investigate next week.

@dlang-bot dlang-bot merged commit 0db2e65 into dlang:master Aug 4, 2020
@Geod24 Geod24 deleted the refactor-cas branch August 4, 2020 13:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants