-
-
Notifications
You must be signed in to change notification settings - Fork 706
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 20502: Converting std.typecons.RefCounted!T to a string gives T's storage location instead of T's fields when T is a struct without an explicit toString #7352
Conversation
|
Thanks for your pull request, @n8sh! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#7352" |
|
Compiling with DMD master prints:
EDIT: This was tested on Win10, run.dlang.io yields the |
|
@MoonlightSentinel I've verified this is still happening with freshly-compiled DMD master with that example verbatim. Pasting the example into https://run.dlang.io/ and running it also gives that result. |
|
Strange, are you on Mac/Posix? |
std/typecons.d
Outdated
| @@ -6544,6 +6559,23 @@ pure @system unittest | |||
| RefCounted!(int*) c; | |||
| } | |||
|
|
|||
| @system unittest // Issue 20502 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for this to be @system other than the cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RefCounted in general is @system.
|
@MoonlightSentinel I've tested this on Windows 10 using dmd v2.090.0. Maybe you had this branch checked out and dmd used it when you ran the example so the fix was already applied. |
I somehow missed that the above behaviour was fixed and thought it was still present. Sorry for the noise |
| // structs that have alias this. | ||
| struct B { int b; alias b this; } | ||
| struct C { B b; alias b this; } | ||
| assert(to!string(refCounted(C(B(123)))) == to!string(C(B(123)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add tests for const/immutable RefCounted (and maybe other storage classes as well)
|
@RazvanN7 Browsing through old issues I came across this PR. Seems to be one of the forgotten PRs, that could be merged (after fixing the trivial merge conflict). In my opinion the request of @MoonlightSentinel for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please fix the merge conflict and we're good to go.
…es T's storage location instead of T's fields when T is a struct without an explicit toString
Example of what this PR fixes: