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.

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

Merged
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

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 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
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
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 2 times, most recently from f2903b7 to 63e4e33 Compare September 21, 2020 19:18
... to ensure that it doesn't throw any exceptions like it used to.
@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.

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.

@@ -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 auto-merge and removed 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
Projects
None yet
4 participants