Bug 7660, incorrect toImpl overload for struct with alias this to class #482

Merged
merged 6 commits into from Apr 21, 2012

Conversation

Projects
None yet
2 participants
@9rnsr

View changes

std/conv.d
@@ -930,7 +930,7 @@ T toImpl(T, S)(S s, in T leftBracket, in T keyval = ":", in T separator = ", ",
/// ditto
T toImpl(T, S)(S s)
- if (is(S : Object) &&
+ if (is(S : Object) && !is(S == struct) &&

This comment has been minimized.

@9rnsr

9rnsr Mar 12, 2012

Member

I think is(S == class) is better than is(S : Object) && !is(S == struct).

@9rnsr

9rnsr Mar 12, 2012

Member

I think is(S == class) is better than is(S : Object) && !is(S == struct).

This comment has been minimized.

@Dav1dde

Dav1dde Mar 12, 2012

Contributor

didn't think of that

@Dav1dde

Dav1dde Mar 12, 2012

Contributor

didn't think of that

@9rnsr

View changes

std/conv.d
+ class C { string toString() { return "C"; } }
+ struct S { C c; alias c this; }
+ S s; s.c = new C();
+ static assert(__traits(compiles, to!string(s)));

This comment has been minimized.

@9rnsr

9rnsr Mar 12, 2012

Member

This static assert is redundant, because the next run-time assertion implicitly needs that itself is compilable.

@9rnsr

9rnsr Mar 12, 2012

Member

This static assert is redundant, because the next run-time assertion implicitly needs that itself is compilable.

This comment has been minimized.

@Dav1dde

Dav1dde Mar 12, 2012

Contributor

I added it to stress, that this bug is fixed and if it occurs again you know it's a regression

@Dav1dde

Dav1dde Mar 12, 2012

Contributor

I added it to stress, that this bug is fixed and if it occurs again you know it's a regression

@9rnsr

View changes

std/conv.d
@@ -959,6 +959,13 @@ unittest
assert(to!string(a) == "null");
a = new A;
assert(to!string(a) == "an A");
+
+ // Bug 7660
+ class C { string toString() { return "C"; } }

This comment has been minimized.

@9rnsr

9rnsr Mar 12, 2012

Member

You need adding override keyword for toString.

@9rnsr

9rnsr Mar 12, 2012

Member

You need adding override keyword for toString.

@9rnsr

View changes

std/conv.d
@@ -930,7 +930,7 @@ T toImpl(T, S)(S s, in T leftBracket, in T keyval = ":", in T separator = ", ",
/// ditto
T toImpl(T, S)(S s)
- if (is(S : Object) &&
+ if (is(S == Object) &&

This comment has been minimized.

@9rnsr

9rnsr Mar 13, 2012

Member

No, == Object and == class are obviously different.
The former returns true only if S equals to Object, and the latter can tests whether S is a kind of class type (not only Object, and user-defined class types).

@9rnsr

9rnsr Mar 13, 2012

Member

No, == Object and == class are obviously different.
The former returns true only if S equals to Object, and the latter can tests whether S is a kind of class type (not only Object, and user-defined class types).

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Mar 13, 2012

Member

Did you run phobos unit test in your local?

Member

9rnsr commented Mar 13, 2012

Did you run phobos unit test in your local?

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Mar 13, 2012

Member

Github doesn't send notify e-mail when you push only commits.
If you add some changes, please add some comment together.

Member

9rnsr commented Mar 13, 2012

Github doesn't send notify e-mail when you push only commits.
If you add some changes, please add some comment together.

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Mar 20, 2012

Member

Ping?

Member

9rnsr commented Mar 20, 2012

Ping?

@Dav1dde

This comment has been minimized.

Show comment
Hide comment
@Dav1dde

Dav1dde Apr 11, 2012

Contributor

(S : Object) → (S == class)

Contributor

Dav1dde commented Apr 11, 2012

(S : Object) → (S == class)

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Apr 11, 2012

Member

Now, you can check your pull's result in Pull Tester.
It shows to you the needing of rebasing.

Member

9rnsr commented Apr 11, 2012

Now, you can check your pull's result in Pull Tester.
It shows to you the needing of rebasing.

@Dav1dde

This comment has been minimized.

Show comment
Hide comment
@Dav1dde

Dav1dde Apr 19, 2012

Contributor

Should work now?
I removed the changelog note, since there's no real place for it (no 2.060 section)

Contributor

Dav1dde commented Apr 19, 2012

Should work now?
I removed the changelog note, since there's no real place for it (no 2.060 section)

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Apr 21, 2012

Member

OK. Changelog conflict is resolved.

Member

9rnsr commented Apr 21, 2012

OK. Changelog conflict is resolved.

9rnsr added a commit that referenced this pull request Apr 21, 2012

Merge pull request #482 from Dav1dde/conv2
Bug 7660, incorrect toImpl overload for struct with alias this to class

@9rnsr 9rnsr merged commit 803aa7c into dlang:master Apr 21, 2012

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