Fix Issue 8747 - isAssignable!(int, const(int)) is false #832

Merged
merged 3 commits into from Oct 4, 2012

Projects

None yet

4 participants

@denis-sh
Contributor
denis-sh commented Oct 3, 2012
  • cause commit: 4a05464
  • add some related checks
  • improve isAssignable unittest
@9rnsr
Member
9rnsr commented Oct 3, 2012

To improve unittest is not so bad, but don't squash unrelated changes into one commit.
You should separate commits which not related to issue 8747.

@9rnsr
Member
9rnsr commented Oct 3, 2012

Changing isAssignable itself looks good to me. After commit separation, I'll merge this.

@andralex andralex commented on the diff Oct 4, 2012
std/traits.d
@@ -2957,8 +2959,7 @@ template isAssignable(Lhs, Rhs)
{
enum bool isAssignable = is(typeof({
Lhs l = void;
- Rhs r = void;
- l = r;
+ void f(Rhs r) { l = r; }
@andralex
andralex Oct 4, 2012 Member

How is this different from the original code?

denis-sh added some commits Oct 4, 2012
@denis-sh denis-sh Improve std.traits unittests
* improve `isAssignable` unittests
* add immutable(S) test to `hasElaborateCopyConstructor`
* add const(S) test to `hasElaborateAssign`
4d2f24d
@denis-sh denis-sh Fix Issue 8747 - isAssignable!(int, const(int)) is false f30660b
@denis-sh
Contributor
denis-sh commented Oct 4, 2012

How is this different from the original code?

Original code:

const(int) ci = void;
int i = ci; // Causes `Error: void initializer has no value` on previous line

By the way, this isAssignable variant also passes unittests:

Lhs l = void;
Rhs r = Rhs.init;
l = r;

But it looks more like a compiler bug:

struct S { @disable this(); @disable this(this); }

void main()
{
    S s1 = S.init; // no errors
    s1 = S.init;   // still no errors
    S s2 = s1; // Error: struct S is not copyable because it is annotated with @disable
}

So, is it a bug?


You should separate commits which not related to issue 8747

It's difficult to distinguish so I decided to separate all not directly related unittest changes. E.g. in hasElaborateCopyConstructor we have this:

S s = void;
return &s.__postblit;

and it may eventually become rejected with void initializer has no value error. So this is related to the source of issue 8747 but not to the issue itself.

@denis-sh
Contributor
denis-sh commented Oct 4, 2012

What about my hasElaborateAssign unittest change? According to docs it's incorrect so either docs should be corrected (S - has elaborate assign, const(S) - hasn't) or implementation bug should be filled and eventually fixed.

@monarchdodra
Collaborator
struct S { @disable this(); @disable this(this); }
void main()
{
    S s1 = S.init; // no errors
    s1 = S.init;   // still no errors
    S s2 = s1; // Error: struct S is not copyable because it is annotated with @disable this
}

That looks like a normal behavior to me, and would be what I expect. S's opAssign is not @disable

struct S { @disable this(); @disable this(this); @disable bool opAssign(this);}
void main()
{
    S s1 = S.init; // no errors
    s1 = S.init;   // THIS TIME Error here
    S s2 = s1;
}

[EDIT] Removed Section


Related: I was creating a pull to fix that "assignmentq" typo. Thanks for getting it.

@monarchdodra
Collaborator

If you are writting a function to do the test (good idea, IMO, btw), then why not just limit the test to the availability of the function itself (as opposed to a block containing a function)?

template isAssignable(T, U)
{
    enum bool isAssignable =
        is(typeof((ref T t, ref U u) => {t = u;}));
}

This form is concise and verbose, and should avoid any traps related to anything disabled.

@9rnsr
Member
9rnsr commented Oct 4, 2012

I think the root cause of bug 8747 is a compiler bug. The original code of isAssignable should work as expected.

@9rnsr
Member
9rnsr commented Oct 4, 2012

@denis-sh: I think this change is enough for fixing the regression. We can merge this before fixing the compiler bug 8753.

@9rnsr
Member
9rnsr commented Oct 4, 2012

You should separate commits which not related to issue 8747

It's difficult to distinguish so I decided to separate all not directly related unittest changes. E.g. in hasElaborateCopyConstructor we have this:

But, the change for isAssignable does not affect to the behavior of hasElaborateCopyConstructor, then they are unrelated.

In any case, your commit separation looks good to me. Let's merge!

@9rnsr 9rnsr merged commit 89ee7f4 into dlang:master Oct 4, 2012

1 check passed

default Pass: 9
Details
@9rnsr
Member
9rnsr commented Oct 4, 2012

Merged.

@andralex
Member
andralex commented Oct 4, 2012

Oh, one more thing. Shouldn't we default the second argument? isAssignable(Lhs, Rhs = Lhs). That way isAssignable!S will tell whether S is assignable from itself.

@9rnsr
Member
9rnsr commented Oct 4, 2012

Oh, one more thing. Shouldn't we default the second argument? isAssignable(Lhs, Rhs = Lhs). That way isAssignable!S will tell whether S is assignable from itself.

Wow, it sounds good. There is undocumented template isIdentityAssignable!T in std.typecons, but we can simply replace it to isAssignable!T.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment