Skip to content

Adds Typedef.toString()#6218

Merged
dlang-bot merged 1 commit intodlang:masterfrom
denizzzka:typedef_tostring
Mar 1, 2018
Merged

Adds Typedef.toString()#6218
dlang-bot merged 1 commit intodlang:masterfrom
denizzzka:typedef_tostring

Conversation

@denizzzka
Copy link
Contributor

@denizzzka denizzzka commented Feb 23, 2018

It is need to avoid silent conversion of Typedef struct itself to string by std.conv.to.

Discussion:
https://forum.dlang.org/post/kjzayldqnixehfprgslc@forum.dlang.org

Previous attempt: #6217

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @denizzzka! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@denizzzka
Copy link
Contributor Author

I am should use git squash or this is not necessary?

@thewilsonator
Copy link
Contributor

Yes do squash. Don't worry about the formatting, see #6220

@wilzbach
Copy link
Contributor

wilzbach commented Feb 25, 2018

Don't worry about the formatting, see #6220

FWIW: This is something different. dmd-cxx is the old C++ branch that didn't compile for years and it's slowly brought back to life (and no one plans to develop with it, we just want to get it to pass on the auto-tester).
Anyhow, while sticking to the DStyle is a requirement for official code, it's not blocking your PR. Every maintainer can easily fix that for you if there's interest in the PR (i.e. the idea has been approved.)


Now back to the PR in question, is there any reason why you didn't add a writer-based toString?
string toString isn't really nice, because it always allocates which is slow.

@JackStouffer has been working hard on adding writer-based toString overloads, even for the mentioned Nullable.toString: #6198

@MetaLang
Copy link
Member

Sorry @denizzzka, I've been busy with other work the past few days and did not have time to properly review this. As @wilzbach pointed out, it would be preferably to add a writer-based overload of toString so there will be no allocation at all when possible. Take a look at the aforementioned pull request he linked to see how to do it.

@denizzzka
Copy link
Contributor Author

Changed for writer support and squashed

}

/// ditto
string toString()
Copy link
Member

Choose a reason for hiding this comment

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

This overload should be documented and considered the "primary" one, with the other just marked as "ditto". Explain that this forwards to the wrapped value and the results between <base type value>.to!string() and <Typedef'd type value>.to!string() will be identical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know how to describe this beautifully, just added an example.

std/typecons.d Outdated
* fmt = A $(REF FormatSpec, std,format) which is used to represent
* the value
* Returns:
* A `string` if `writer` and `fmt` are not set; `void` otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect, as each overload will show up separately in the documentation, and it will confuse readers if you say that this one could return void or string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(This is copy and paste from Nullable, so its documentation is weak too)

std/typecons.d Outdated
assert(s2 == cs2);
}

@safe unittest // toString
Copy link
Member

Choose a reason for hiding this comment

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

More unit tests are needed that test both overloads of toString with multiple different types of values.

Another good test to add would be something like the following to be reasonably sure that the results of Typedef.toString are the same as the base type:

import std.meta: AliasSeq;
import std.conv;

struct TestS {}
class TestC {}

static foreach (T; AliasSeq!(int, bool, float, double, real, ifloat, idouble, ireal,
                             cfloat, cdouble, creal, char, dchar, wchar, TestS, 
                             TestC, int*, int[], int[2], int[int]))
{{
    T t;
    Typedef!T td
    assert(t.to!string() == td.to!string());
}}

@denizzzka
Copy link
Contributor Author

Something strange with CI check

@wilzbach
Copy link
Contributor

wilzbach commented Mar 1, 2018

Not too strange though.
Just follow the advice of the message and add @safe to your unittest. Explicit annotation is required because it's too often forgotten.

@denizzzka
Copy link
Contributor Author

Oh, thank you, did not noticed by myself.

@dlang-bot dlang-bot merged commit df68d4c into dlang:master Mar 1, 2018
@MetaLang
Copy link
Member

MetaLang commented Mar 1, 2018

Thanks @denizzzka!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants