Skip to content

Modified the behavior of std.stdio.LockingTextWriter.put #6092

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.

Sign up for GitHub

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

Merged
dlang-bot merged 1 commit into from
Jan 30, 2018
Merged

Modified the behavior of std.stdio.LockingTextWriter.put #6092

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions std/stdio.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2880,8 +2880,7 @@ $(D Range) that locks the file and allows fast writing to it.
}

// put each element in turn.
alias Elem = Unqual!(ElementType!A);
foreach (Elem c; writeme)
foreach (c; writeme)
{
put(c);
}
Expand All @@ -2908,7 +2907,7 @@ $(D Range) that locks the file and allows fast writing to it.
}
else static if (c.sizeof == 2)
{
import std.utf : encode, UseReplacementDchar;
import std.utf : encode;

if (orientation_ <= 0)
{
Expand All @@ -2919,7 +2918,7 @@ $(D Range) that locks the file and allows fast writing to it.
else
{
char[4] buf;
immutable size = encode!(UseReplacementDchar.yes)(buf, c);
immutable size = encode(buf, c);
Copy link
Contributor

@wilzbach wilzbach Jan 29, 2018

Choose a reason for hiding this comment

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

I saw there was some discussions at #6091 (comment) (for the future - it would be great if you could either re-use the PR or reference such discussions at the releveant at the new PR as it took me a while to find the discussions and I almost missed it).

The annoyance that auto-decoding makes strings neither nothrow nor @nogc (though the undocumented -dip1008 for `@nogc~ exceptions has been merged and might soon be enabled by default) should make us really put an end to the auto-decoding hell.

FWIW there has been DIP76 once:

When the autodecoder encounters invalid UTF, it throws an exception. This proposal changes that so it replaces the invalid UTF with the Replacement Character.

I guess that one never went anywhere :/

Anyhow I'm still not sure I understand the motivation for this change.
Also it could have resulted in a breaking change as this could have changed the function from nothrow to throws.
It doesn't due to the nasty _handle having it's attributes specified manually and thus not being nothrow nor @nogc:

phobos/std/stdio.d

Line 2825 in afa7d58

@property _iobuf* handle_() @trusted { return cast(_iobuf*) file_._p.handle; }

but I would like to see such things pointed out by the submitter and not needing to dig them out myself :/
There can also be a case made that the dchar version doesn't use the UseReplacementDchar version, but again the one proposing the change should bring up the arguments for it.

Copy link
Member

@quickfur quickfur Jan 29, 2018

Choose a reason for hiding this comment

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

Maybe it's time for us to make another big push to kill autodecoding for good. It's been almost universally hated by all, and while we did finally manage to convince Andrei the last time round that it's actually a bad thing (after many years of interminable arguments), we didn't go far enough, IMO -- we only got as far as introducing byCodeUnit, et al, and to try to reduce reliance on autodecoding in various Phobos functions.

But it has been an ongoing source of performance concerns, and now nothrow and @nogc unfriendliness, that perhaps we can make a very strong case for getting rid of it forever. I, for one, plan to celebrate the day the last of autodecoding is finally killed off from Phobos code forever.

Copy link
Contributor Author

@JackStouffer JackStouffer Jan 29, 2018

Choose a reason for hiding this comment

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

@quickfur Did you see this comment #6073 (comment) ?

Copy link
Member

@quickfur quickfur Jan 30, 2018

Choose a reason for hiding this comment

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

RCStr has been talked about since, what, a year ago or more? When is it ever going to materialize? What's the holdup?

Copy link
Contributor Author

@JackStouffer JackStouffer Jan 30, 2018

Choose a reason for hiding this comment

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

@quickfur DIP1000. I stalled Walter's PRs for it because I believed it wasn't handling GC-ed objects correctly (and Martin agreed #5052 (comment)). There's been no work on DIP1000 since my comments on the matter so I think Walter dropped it for some reason.

Copy link
Contributor

@wilzbach wilzbach Jan 30, 2018

Choose a reason for hiding this comment

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

Here's a list of what modules aren't compatible with DIP1000 as of now: https://github.com/dlang/phobos/projects/6

There has been some recent work on DIP1000 at #5915 and #6041. The first one stalled because DIP1000 uses a different mangling :/

Copy link
Contributor Author

@JackStouffer JackStouffer Jan 30, 2018

Choose a reason for hiding this comment

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

There has been some recent work on DIP1000 at #5915 and #6041

Ok, there's been no work by Walter since

Copy link
Member

@quickfur quickfur Jan 30, 2018

Choose a reason for hiding this comment

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

While dip1000 isn't perfect by any stretch, I still would welcome it as an improvement on the current situation.

foreach (i ; 0 .. size)
trustedFPUTC(buf[i], handle_);
}
Expand Down Expand Up @@ -2981,10 +2980,15 @@ $(D Range) that locks the file and allows fast writing to it.
}
}

/** Returns an output range that locks the file and allows fast writing to it.

See $(LREF byChunk) for an example.
*/
/**
* Returns: An $(REF_ALTTEXT output range, isOutputRange, std, range, primitives)
* that locks the file and allows fast writing to it.
*
* Throws: $(REF UTFException, std, utf) if the data given is a `char` range
* and it contains malformed UTF data.
*
* See_Also: $(LREF byChunk) for an example.
*/
auto lockingTextWriter() @safe
{
return LockingTextWriter(this);
Expand Down