Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue 20138 - is expression not evaluating correctly? #10317

Merged
merged 4 commits into from Aug 20, 2019

Conversation

aG0aep6G
Copy link
Contributor

When passing a shared const T into a shared U parameter, then no const
conversion is needed to infer U as const int. Same with inout int and
inout const int.

When passing a `shared const T` into a `shared U` parameter, then no const
conversion is needed to infer `U` as `const int`. Same with `inout int` and
`inout const int`.
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @aG0aep6G! 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
20138 blocker is expression not evaluating correctly?

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

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 + dmd#10317"

@TurkeyMan
Copy link
Contributor

I don't know how to review this...

@aG0aep6G
Copy link
Contributor Author

PR to prepare Phobos for this fix: dlang/phobos#7147

@aG0aep6G
Copy link
Contributor Author

For reference, dlang/druntime#2739 seems to be blocked by this.

@Biotronic
Copy link
Contributor

It seems to me this fix only deals with shared T, and ignores other qualifiers - is(shared const int U == const U) should also be true (and U should be shared int), likewise for inout.

@aG0aep6G
Copy link
Contributor Author

It seems to me this fix only deals with shared T, and ignores other qualifiers - is(shared const int U == const U) should also be true (and U should be shared int), likewise for inout.

Right. Added those cases.

@thewilsonator
Copy link
Contributor

Phobos PR merged.

@TurkeyMan
Copy link
Contributor

Force rebuild?

alias S = shared int;
static assert(is(const(S) U == const U) && is(U == shared int));
static assert(is(inout(S) U == inout U) && is(U == shared int));
static assert(is(inout(const S) U == inout(const U)) && is(U == shared int));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding static assert(is(inout const S U == inout U) && is(U == const shared S));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggest adding static assert(is(inout const S U == inout U) && is(U == const shared S));

I'd rather leave that out for now, because it's less obvious how to handle it.

For the others, I just changed MATCH.constant to MATCH.exact. For this combination, deduceTypeHelper currently returns MATCH.nomatch. But it works in IFTI, so there must be something else going on. My guess is that it's handled by deduceTypeHelper, which isn't even called for is expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is that it's handled by deduceTypeHelper, which isn't even called for is expressions.

That was supposed to be: My guess is that it's handled by deduceWildHelper

Copy link
Contributor

Choose a reason for hiding this comment

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

I was gonna say we could do something like this:

    *at = t.mutableOf().unSharedOf();
    if ((t.mod & MODFlags.const_) && !(tParam.mod & MODFlags.const_))
        *at = (*at).constOf();
    if ((t.mod & MODFlags.shared_) && !(tParam.mod & MODFlags.shared_))
        *at = (*at).sharedOf();
    if ((t.mod & MODFlags.immutable_) && !(tParam.mod & MODFlags.immutable_))
        *at = (*at).immutableOf();
    if ((t.mod & MODFlags.wild) && !(tParam.mod & MODFlags.wild))
        *at = (*at).wildOf();

However, apparently calling sharedOf() on a const type doesn't return a const shared type, and likewise for the other *Of()s. This somewhat disappoints me.

@aG0aep6G
Copy link
Contributor Author

I'm not sure if it's really necessary to mention this in the changelog, but @thewilsonator has requested an entry, so I've added one.

@TurkeyMan
Copy link
Contributor

Is this good?

@thewilsonator
Copy link
Contributor

so I've added one.

Thanks. I wanted it because this is a non-obvious change of behaviour that would be better documented than not. It probably won't break anyones code, but, OTOH it did require a change in phobos.

@dlang-bot dlang-bot merged commit 80b05b0 into dlang:master Aug 20, 2019
@aG0aep6G aG0aep6G deleted the 20138 branch August 20, 2019 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants