This is the feedback thread for the first round of Community Review for DIP 1024, "Shared Atomics":

https://github.com/dlang/DIPs/blob/0b892dd99aba74b9631572ad3a53000f5975b7c2/DIPs/DIP1024.md

All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on October 15, or when I make a post declaring it complete.

At the end of Round 1, if further review is deemed necessary, the DIP will be scheduled for another round of Community Review. Otherwise, it will be queued for the Final Review and Formal Assessment.

Anyone intending to post feedback in this thread is expected to be familiar with the reviewer guidelines:

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

**Thanks in advance for keeping all discussion on topic.**

This DIP will need to detail how it does (and doesn't) interact with methods with the shared attribute on it, along with examples of the shared qualifier on a struct/class.

As it stands, if I was to implement it, I could interpret the DIP to only affect globals.

On Tuesday, 1 October 2019 at 10:40:52 UTC, Mike Parker wrote:
> [snip]

In terms of other languages that might take a similar approach, the DIP only mentions C's volatile. The Task Parallelism and Synchronization section of the Chapel Language specification [1] might be of interest.

[1] https://chapel-lang.org/docs/_downloads/chapelLanguageSpec.pdf

On Tuesday, 1 October 2019 at 10:40:52 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1024, "Shared Atomics":

Section "Alternatives" should be properly referenced as it makes unsubstantiated claims.(Also the references are not linked to from the main document).

The linked implementation (which is already merged(!)) appears to bear no correlation to the DIP, which appears to suffer a severe identity crisis, and as such completely fails the vengeful-ex principle:

> This change will require using core.atomic or equivalent functions to read and write to shared memory objects. It will prevent unintended, inadvertent non-use of atomic access.
> ...
> By prohibiting direct access to shared data, the user will be required to use core.atomic and to consider the correctness of their code.

> [The DIP is] just replacing calls to core.atomic with language support...
> This is not pioneering new ground. The only innovation is support by the language type system rather than library calls.
> x=1; // atomic store, release, happens after everything above

So which one is it?
If it is the first, all the references to apparent automatic rewriting should be dropped.
If it is the second, I fail to see why this is semantically any different to what C++ does with atomic<int>, which the DIP makes the claim is controversial.

On Tuesday, 1 October 2019 at 10:40:52 UTC, Mike Parker wrote:
>
>"This change will require using core.atomic or equivalent functions to read and >write to shared memory objects. It will prevent unintended, inadvertent non-use of >atomic access."

What if the type is complex (like a struct) outside what the ISA can handle? Usually this only applies for integers 8 - 128 bits depending on architecture, anything beyond that is out of reach unless you want to dig into Intel TSX which the shared attribute isn't suitable for.

Another thing that is left out are the effects of operator overloading.

shared int s;

// What would this do? Atomically increment s or just atomically load s, add one
// and then atomically store s?
s = s + 1;
// Would this atomically increase s and return the value after increment?
s++;

On Tuesday, 1 October 2019 at 10:40:52 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1024, "Shared Atomics":
>
> [...]

There is no detail on how is this going to be implemented.

-Alex

On Tuesday, 1 October 2019 at 10:40:52 UTC, Mike Parker wrote:

[...]

My feedback: What is the proposal?

"By prohibiting direct access to shared data, the user will be required to use core.atomic and to consider the correctness of their code."

"This change will require using core.atomic or equivalent functions to read and write to shared memory objects. It will prevent unintended, inadvertent non-use of atomic access."

"Since it's just replacing calls to core.atomic with language support, and atomics are well understood (at least by experts) in multiple languages, there should be little risk. This is not pioneering new ground. The only innovation is support by the language type system rather than library calls."

It's possible that I just don't understand this topic well enought, but in my opinion it's more of a sketch of things that should be discussed in a DIP rather than a DIP. An example would go a long way.

On Tuesday, 1 October 2019 at 16:22:36 UTC, bachmeier wrote:
> My feedback: What is the proposal?

This is the proposal (first paragraph of the DIP / Abstract): "Reads and writes to data typed as shared are made atomic where the target CPU supports atomic operations and become erroneous when they cannot be done atomically."

The rest of the DIP sometimes seems to contradict the Abstract (e.g. "prohibiting direct access to shared data", "will require using core.atomic"). It should probably be clarified that using core.atomic will only be necessary when the compiler can't generate the atomic operation itself for whatever reason.

On Tuesday, 1 October 2019 at 16:49:25 UTC, ag0aep6g wrote:
> [snip]
>
> This is the proposal (first paragraph of the DIP / Abstract): "Reads and writes to data typed as shared are made atomic where the target CPU supports atomic operations and become erroneous when they cannot be done atomically."
>
> The rest of the DIP sometimes seems to contradict the Abstract (e.g. "prohibiting direct access to shared data", "will require using core.atomic"). It should probably be clarified that using core.atomic will only be necessary when the compiler can't generate the atomic operation itself for whatever reason.

I assumed bachmeier's point was more an issue with the organization and presentation of the DIP than what the proposal actually is...

On 10/1/2019 4:21 AM, rikki cattermole wrote:
> This DIP will need to detail how it does (and doesn't) interact with methods with the shared attribute on it, along with examples of the shared qualifier on a struct/class.

The DIP does not change how `shared` is part of the type system, and does not change how `shared` affects the types of structs or classes or instances of them.

> As it stands, if I was to implement it, I could interpret the DIP to only affect globals.

`shared` isn't a storage class, it's a type constructor, so I don't see how you could infer that.