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.

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

Jump to bottom

fix issue 18789 - std.stdio messes up UTF conversions on output #6469

Merged
merged 5 commits into from
Oct 6, 2020
Merged

fix issue 18789 - std.stdio messes up UTF conversions on output #6469

merged 5 commits into from
Oct 6, 2020

Conversation

aG0aep6G
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Apr 21, 2018 •
edited
Loading

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 coverage diff by visiting the details link of the codecov check)
  • 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 run digger -- build "master + phobos#6469"

@aG0aep6G aG0aep6G changed the title fix issue 18789 - std.stdio messes up UTF conversions on output [WIP] fix issue 18789 - std.stdio messes up UTF conversions on output Apr 22, 2018
@dlang-bot dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Apr 22, 2018
@aG0aep6G
Copy link
Contributor Author

I think I'm soon ready to give up on this. Latest surprise: Microsoft's fwide is useless ("unimplemented"). So orientation_ is bogus on that platform. With Microsoft, the distinction is ANSI/Unicode, not narrow/wide.

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.

@aG0aep6G aG0aep6G mentioned this pull request Apr 23, 2018
Merged
@aG0aep6G
Copy link
Contributor Author

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.

#6474

@aG0aep6G aG0aep6G force-pushed the LockingTextWriter.put branch from 6533e95 to d2e375e Compare September 17, 2020 22:22
@aG0aep6G
Copy link
Contributor Author

Rebooted without out the stuff that has already been merged and the debugging commits. Let's see where it breaks this time around.

@aG0aep6G
Copy link
Contributor Author

I think we need to clarify what LockingTextWriter's mission statement is. Is it (A) print text in a readable form (to a terminal)? Or is it (B) write UTF-8 always?

If A, we should always enforce wide/Unicode mode, transcode to wchar_t, and call FPUTWC. This would fix writeln("Hello, wörld!"); on MICROSOFT_STDIO Windows, without needing chcp 65001 or similar nonsense. I don't know if DIGITAL_MARS_STDIO Windows can be made to work, because it doesn't seem to have a Unicode mode.

If B, we should always enforce binary mode, transcode to UTF-8, and call FPUTC.

If neither, what is LockingTextWriter's mission?

@aG0aep6G
Copy link
Contributor Author

I think we need to clarify what LockingTextWriter's mission statement is.

Ignoring that, I've rewritten the unittest. Might pass now.

@aG0aep6G aG0aep6G force-pushed the LockingTextWriter.put branch from 62b0f2e to 978dafb Compare September 21, 2020 19:06
@aG0aep6G aG0aep6G force-pushed the LockingTextWriter.put branch 2 times, most recently from f2903b7 to 63e4e33 Compare September 21, 2020 19:18
@aG0aep6G
add the original test case of issue 18789
e7e75cf 
... to ensure that it doesn't throw any exceptions like it used to.
@aG0aep6G aG0aep6G force-pushed the LockingTextWriter.put branch from 63e4e33 to e7e75cf Compare September 21, 2020 19:19
@aG0aep6G
Copy link
Contributor Author

I think this is finally ready for review. buildkite says druntime is failing, but that seems unrelated.

@aG0aep6G aG0aep6G changed the title [WIP] fix issue 18789 - std.stdio messes up UTF conversions on output fix issue 18789 - std.stdio messes up UTF conversions on output Sep 21, 2020
@aG0aep6G
Copy link
Contributor Author

aG0aep6G commented Oct 5, 2020

I think this is finally ready for review.

Still waiting for someone to take a look at this.

thewilsonator
thewilsonator approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Otherwise looks good.

std/stdio.d
@@ -3640,6 +3667,69 @@ void main()
}
assert(std.file.readText!string(deleteme).stripLeft("\uFEFF") == "foobar");
}
@safe unittest // char/wchar -> wchar_t
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough.

@thewilsonator thewilsonator added Merge:auto-merge and removed Review:WIP Work In Progress - not ready for review or pulling labels Oct 5, 2020
@aG0aep6G
Copy link
Contributor Author

aG0aep6G commented Oct 6, 2020

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?

@wilzbach wilzbach merged commit 8f2af24 into dlang:master Oct 6, 2020
@wilzbach
Copy link
Member

wilzbach commented Oct 6, 2020

Yeah it's a known issue that sometimes the wrong druntime gets checked out

@aG0aep6G aG0aep6G deleted the LockingTextWriter.put branch October 6, 2020 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thewilsonator thewilsonator thewilsonator approved these changes

@CyberShadow CyberShadow Awaiting requested review from CyberShadow CyberShadow is a code owner

@schveiguy schveiguy Awaiting requested review from schveiguy schveiguy is a code owner

Assignees
No one assigned
Labels
Merge:auto-merge Severity:Bug Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aG0aep6G @dlang-bot @wilzbach @thewilsonator