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

to!SomeString should use formatValue #236

Merged
merged 7 commits into from
Oct 31, 2011
Merged

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Sep 5, 2011

Pull #126 was now merged, so we can remove code duplication between std.conv.to and std.format.formatValue.

@andralex
Copy link
Member

andralex commented Sep 5, 2011

Oh okay. Can you think of another case where performance may suffer? We don't have good benchmarking for the time being, and I wouldn't want to pull in something that may slow things down.

@9rnsr
Copy link
Contributor Author

9rnsr commented Sep 5, 2011

I have reverted only removing dup/idup optimization, see 8b3f98b.
As far as I see, only it was the lacking from my performance perspective.


Additionally, it seems to me that the to!String family is convenient function set, not intended to high performance.
The almost of them requires heap allocation to return new string, so that is essentially equals to using appender.
And, maybe, using appender is slower than array concatenation, but I think it is druntime issue, not Phobos issue.
(I am aware of the current Appender implementation is workaround.)

Finally, indeed, this change will make to!String a bit slower, by a little overhead like FormatSpec, but it seems to me that is less important than removing code duplication.

@andralex
Copy link
Member

This looks okay, but we need better deprecation messages than "schedule for deprecation". Kenji, could you please look at how deprecation is handled in other modules and do the same here? Thanks!

@9rnsr
Copy link
Contributor Author

9rnsr commented Sep 29, 2011

Rebased and improved soft deprecation message (2.056 release == November 2011 + 6 months).

andralex added a commit that referenced this pull request Oct 31, 2011
to!SomeString should use formatValue
@andralex andralex merged commit 85c3586 into dlang:master Oct 31, 2011
kuettler pushed a commit to kuettler/phobos that referenced this pull request Feb 6, 2018
Rename update.sh to setup.sh and make it work with auto-boostraping
merged-on-behalf-of: Vladimir Panteleev <github@thecybershadow.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants