fix conversion from wchar to char in LockingTextWriter.put

[aG0aep6G]

Spin-off from #6469. I'm hoping that this will be relatively easy to get to work on the different platforms.

Instead of throwing a UTFException on unpaired high surrogates, a replacement character could be written. But throwing is what happens currently for unpaired low surrogates, so I went with that.

[dlang-bot]

Thanks for your pull request and interest in making D better, @aG0aep6G! 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

Auto-close	Bugzilla	Severity	Description
✗	18789	normal	std.stdio messes up UTF conversions on output

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6474"

[aG0aep6G]

I'm hoping that this will be relatively easy to get to work on the different platforms.

Yup. All green.

[schveiguy]

Aside from the fact that you are adding nice checks for partially written wchar surrogate pairs, when you aren't doing the same for char encodings, this looks pretty good. All of my suggestions are optional, though I would like to see the performance improvements.

[schveiguy]

What happens if you don't include this?

[aG0aep6G]

The readText below sees an empty file.

And now I see why: LockingTextWriter holds a reference counted File. When the destructor throws, the File won't be destroyed and its reference count won't be decremented. So the file only gets closed at the end of the program.

Fixed by destroying file_ explicitly before throwing.

[schveiguy]

Ah, glad you figured that out!

[schveiguy]

Took me a few minutes to understand that the first assertThrown above is testing the throw from the lockingTextWriter dtor. Can you put in a comment to that effect?

[aG0aep6G]

Added a comment at the end of the block.

[schveiguy]

This seems like a lot of acrobatics for this purpose. A few comments here:

1. You only need a buffer of 1 wchar.
2. You don't need a "size" for checking if there are 0 or 1 wchars in there, just store a wchar(0) when you aren't using it.
3. From my experience with iopipe, you get better performance when you avoid using members to do the decoding. This would be better off being duplicated in the function which takes whole wstrings, and you can then use a local to do all the decoding/encoding.

[aG0aep6G]

Addressed points 1 and 2. Hopefully, the code's clearer now.

I think I'll leave point 3 for another PR.

[schveiguy]

Good progress on the simplification. One more thing, I thought it might look better actually if you handle the surrogate pair explicitly instead of the ?: operator

else
{
   dchar d = c; // default to just translate directly
   if(highSurrogate)
   {
      immutable wchar[2] buf = [highSurrogate, c];
      d = buf[].front; // doesn't this just work? Do we need to use decodeFront?
      highSurrogate = 0;
   }
   // .. all the rest of the stuff you have
}

[schveiguy]

It would be cool if std.utf provided a way to decode N code units directly. For example, if decode(highSurrogate, c) just worked.

[aG0aep6G]

Your code misses the case where c is an unpaired low surrogate. But encode throws on that, so that works. Done.

[aG0aep6G]

you are adding nice checks for partially written wchar surrogate pairs, when you aren't doing the same for char encodings

If I find the spiritual strength to continue with #6469, I should add checks for UTF-8, too, yeah.

[schveiguy]

I suppose the difference between wchar and char is that LockingTextWriter is actually writing char, so there is a one-to-one mapping. When you are writing wchar, you can't just stop in the middle of a surrogate pair because nothing has been written. When you are writing char, you can write everything given. In any case, there is no denying that unicode is messy when it comes to streaming.

[aG0aep6G]

I suppose the difference between wchar and char is that LockingTextWriter is actually writing char, so there is a one-to-one mapping. When you are writing wchar, you can't just stop in the middle of a surrogate pair because nothing has been written. When you are writing char, you can write everything given.

Yeah, I wouldn't validate in the char -> char case. Better just pass it through.

But there's also the char -> wchar_t case. There, the situation is the same as here, and I should check for unused chars in the buffer before starting a new code point.

[schveiguy]

OK, I think this looks good. I'll give it a bit of time, and then merge it. @wilzbach does that label do it automatically?

[JackStouffer]

No, it's manual.

[schveiguy]

Not sure why the bot isn't merging, but I'll do it via auto-tester. Ping @wilzbach

[schveiguy]

Auto-merge toggled on

[wilzbach]

Because the bot only merges if all CI pass and Jenkins failed here.

[schveiguy]

Ah, ok. Jenkins wasn't marked as required, so I assumed it would merge.

[wilzbach]

Yeah the default behavior has been changed a few month ago as it's better if a human judges spurious CI failures and also it's better if we don't have them at all.
In this case, the failure was in DCD with this PR hopefully fixing this
dlang-community/DCD#470
(and I also temporarily removed DCD from the tester until the PR is part of the next release - dlang-community/DCD#455)