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

core.atomic.cas: add 'const' attribute to local variable 'arg1' #3373

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

thaven
Copy link
Contributor

@thaven thaven commented Feb 12, 2021

Avoid possible unnecessary const to mutable conversion, which is not implicit for pointers. See Issue 21631 for details.

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 12, 2021

Thanks for your pull request and interest in making D better, @thaven! 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

Auto-close Bugzilla Severity Description
21631 normal core.atomic.cas fails to compile with const ifThis (if target is a pointer)

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

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Feb 12, 2021
@thaven thaven force-pushed the bugfix/21631-cas-const-ifThis branch from 5fe1877 to b748ecd Compare February 12, 2021 12:46
@RazvanN7
Copy link
Contributor

This seems to break both the dmd's and phobos's test suites.

@Geod24
Copy link
Member

Geod24 commented Feb 22, 2021

That looks like an invalid issue to me. You should not be able to store a const pointer in a non-const pointer.
The code is essentially trying to do the following:

const(int)* ptr;
int* ptr = ptr;

Which is not sound.

@thaven
Copy link
Contributor Author

thaven commented Feb 22, 2021

@Geod24 not really. The case is more like:

int* a;
const(int)* b;
int* c;

if (a is b)
    a = c;

So it is just comparing pointer to const against pointer to mutable.

@Geod24
Copy link
Member

Geod24 commented Feb 22, 2021

Oh, I see. Indeed. However using const is not going to cut it. Those "implicit conversions" statements IMO are wrong and shouldn't be there. But you'd have to dig the git history to see why they are there in the first place.

@thaven
Copy link
Contributor Author

thaven commented Feb 22, 2021

Those statements seem to originate from this commit: 1b13903 from PR #2745

@RazvanN7
Copy link
Contributor

ping @thaven . How do we proceed here?

@thaven thaven force-pushed the bugfix/21631-cas-const-ifThis branch from b748ecd to 2c97de0 Compare August 12, 2021 12:16
@dlang-bot dlang-bot removed the stalled label Aug 12, 2021
@thaven
Copy link
Contributor Author

thaven commented Aug 12, 2021

It all depends on what to do about the assignments to 'resolve implicit conversions'. I guess those are there to get more clear error messages in case of a type error related to cas arguments. Maybe @TurkeyMan can shed some light on that.

Anyway, the mere presence of those lines doesn't cause the issue, it's the wrong type on arg1.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Feb 7, 2022

This seems to be passing all tests now.

@dlang-bot dlang-bot removed the stalled label Feb 7, 2022
@dkorpel dkorpel force-pushed the bugfix/21631-cas-const-ifThis branch from 2c97de0 to b46844a Compare March 28, 2022 11:21
@dlang-bot dlang-bot merged commit 14e1554 into dlang:master Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants