[core.atomic] atomicStore for alias this

[thewilsonator]

@kinke the corresponding LDC commit mentions some unit tests, but I presume that they are in the main LDC repo, but I can't fin the correspond PR/commit. Do you know where they are and should they be moved to druntime?

[dlang-bot]

Thanks for your pull request and interest in making D better, @thewilsonator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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#3052"

[kinke]

I couldn't find the unittest after a quick search either, maybe it has been removed in the meantime. Anyway, a simple test that works for LDC but not DMD due to this would be:

unittest
{
    static struct S
    {
        int a;
        alias a this;
    }

    import core.atomic;
    S s;
    atomicStore(s, 123);
}

[thewilsonator]

Awesome. Thanks!

[thewilsonator]

Tests added.

[Geod24]

Do we really need this implicit conversion magic though ? The argument is passed by ref, implicit conversions are not expected. I'd argue that anyone that wants to do this should specify T explicitly on the caller side.
If you really want implicit conversions, the signature should reflect it, e.g. T should be deduced to be the common acceptable type.

[thewilsonator]

Git blames #2745 for the comment // resolve implicit conversions
cc'ing @TurkeyMan

[TurkeyMan]

My refactor to simplify and untangle the implementation expects that all weird type wrangling is in the front-end wrappers. The internal functions deal only with one type, and should be the only functions required for the implementations to fork.

"Implicit conversion" is for cases like T == int and V == short. Or T == Base* and V == Derived*.

The case in question with alias this feels weird to me. I'm not sure I call that an 'implicit conversion'... that's something else, and I don't know how deep that rabbit hole goes.
I think I'd rather make a patch to exclude this case (leading to compile error) rather than trying to handle it like this.

Should we add the constraint is(V : T) to the function?

[RazvanN7]

@Geod24 Since alias this is a language feature I don't see why we should not support it in this case. I wouldn't call it implicit conversion since the user explicitly defined the alias this.

I agree that a better solution would be to stress out the condition in the template constraint so that in case of screw ups the compiler doesn't point to the innards of atomicStore.

@thewilsonator Please update the function signature so that we can get this in.

[thewilsonator]

Please update the function signature so that we can get this in.

To what? && is(V : T)?

[RazvanN7]

To what? && is(V : T)?

Yes.

[thewilsonator]

Done..

[RazvanN7]

Thanks!

[thewilsonator]

Hmm, this probably needs a rebase

[RazvanN7]

Nope, the error is legit. You probably need to switch the types of the is expression: is(T : V)

[thewilsonator]

Sorry, seems I missed this. Suggestion applied. Lets see if this works.

[RazvanN7]

This doesn't seem to be going anywhere. Also, pondering upon it, I agree with @Geod24 : "Do we really need this implicit conversion magic though ? The argument is passed by ref, implicit conversions are not expected.". Feel free to reopen if you still want to pursue this.