-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
fix issue 18789 - std.stdio messes up UTF conversions on output #6469
Conversation
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 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
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#6469" |
I think I'm soon ready to give up on this. Latest surprise: Microsoft's I can probably split off the wchar -> char case, though. That doesn't involve any file mode madness, as far as I can see. I'm going to look into that tomorrow. |
|
6533e95
to
d2e375e
Compare
Rebooted without out the stuff that has already been merged and the debugging commits. Let's see where it breaks this time around. |
I think we need to clarify what If A, we should always enforce wide/Unicode mode, transcode to If B, we should always enforce binary mode, transcode to UTF-8, and call If neither, what is |
Ignoring that, I've rewritten the unittest. Might pass now. |
62b0f2e
to
978dafb
Compare
f2903b7
to
63e4e33
Compare
... to ensure that it doesn't throw any exceptions like it used to.
63e4e33
to
e7e75cf
Compare
I think this is finally ready for review. buildkite says druntime is failing, but that seems unrelated. |
Still waiting for someone to take a look at this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks good.
@@ -3640,6 +3667,69 @@ void main() | |||
} | |||
assert(std.file.readText!string(deleteme).stripLeft("\uFEFF") == "foobar"); | |||
} | |||
@safe unittest // char/wchar -> wchar_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for this unittest to not just be @system
, what with all the @trusted
lambdas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing that's actually being tested is writer.put(s)
. And we might want to ensure that that's @safe
. The @trusted
parts are just setup for the test.
But truth be told, I just started with @safe
and added @trusted
as needed. I can change it to @system
if you like that better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough.
I guess auto-merge doesn't work when buildkite is red? As far as I can tell, this is the error in druntime: generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D3std5regex8internal9kickstart__T7ShiftOrTaZQl6__ctorMFNcNeKSQCiQChQCe2ir__T5RegexTaZQjAkZSQDmQDlQDiQDc__TQCvTaZQDb: error: undefined reference to '_D3std5range__T11SortedRangeTAkVAyaa6_61203c3d2062VEQByQBx18SortedRangeOptionsi0ZQCo6lengthMFNaNbNdNiNfZm' I can't reproduce that locally. And I don't see how this PR could cause that undefined reference. What do I do now? |
Yeah it's a known issue that sometimes the wrong druntime gets checked out |
No description provided.