Skip to content

Conversation

9il
Copy link
Member

@9il 9il commented Nov 15, 2014

Blocked by DMD Issue: https://issues.dlang.org/show_bug.cgi?id=13745
History of this PR with commits: https://github.com/9il/phobos/tree/move2
See also: #2737

The reason is to do not import std.uni tables, std.string and
probably std.algorithm.

Public import format added in std.string with docs like icmp.

@@ -1503,8 +1503,6 @@ private auto packedArrayView(T)(inout(size_t)* ptr, size_t items) @trusted pure
// Partially unrolled binary search using Shar's method
//============================================================================

private import std.math : pow;
Copy link
Member

Choose a reason for hiding this comment

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

Not used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. ^^ instead.

Copy link
Member

Choose a reason for hiding this comment

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

Last time I checked it required std.math.pow maybe it doesn't now...

Copy link
Member Author

Choose a reason for hiding this comment

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

it doesn't

@@ -576,7 +576,7 @@ unittest
{
import std.functional;
import std.algorithm : find;
import std.uni : isWhite;
import std.ascii : isWhite;
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

To do not import std.uni. For this unittest any isWhite is good.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a bit extreme. This is a unittest, not a function everyone is going to use. Why does an import inside a unittest matter so much? If it's actually a problem, then we need to be addressing the root cause of the problem (on the forum) instead of just randomly removing things from Phobos unittests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not matter =) I will remove this change after other recommendations will be given.

Copy link
Member Author

Choose a reason for hiding this comment

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

other recommendations - it is about all PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's avoid the use of std.ascii in the documented unittest, no need to encourage avoiding Unicode.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

@DmitryOlshansky
Copy link
Member

Otherwise LGTM. The comment about sformat vs format is better left for future PRs.

The reason is to do not import `std.uni` tables, `std.string` and
probably `std.algorithm`. Note that format is used in CTFE code and
`Exception` handing.

 And it is more comfortable to import `format` from `std.format`.

std.format: clean imports (2)

remove import std.math : pow from std.uni

update scope imports in std.algorithm

update scope imports in std.exception

doFormat -> template

update scope imports in std.typecons

update scope imports in std.functional

update scope imports in std.range

update std.conv scope import

std.format: clean imports (2)

remove import std.math : pow from std.uni

update scope imports in std.algorithm

update scope imports in std.exception

doFormat -> template

update scope imports in std.typecons

update scope imports in std.functional

update scope imports in std.range

move sfromat

add public import of sformat

use std.uni
@9il
Copy link
Member Author

9il commented Nov 18, 2014

ascii fixed. Rebased.

@quickfur
Copy link
Member

LGTM, thanks!

@quickfur
Copy link
Member

On second thoughts, what about DMD bug https://issues.dlang.org/show_bug.cgi?id=13745? Will this move cause problems because of that bug?

@9il
Copy link
Member Author

9il commented Nov 18, 2014

The unittests pass until changes in UTFException. So it can be merged.
But we should keep this PR in mind.

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

DmitryOlshansky added a commit that referenced this pull request Nov 18, 2014
move format to std.format
@DmitryOlshansky DmitryOlshansky merged commit f5a53ad into dlang:master Nov 18, 2014
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.

3 participants