Thanks for the review@gasche! I agree that my approach may be more complex/error-prone than needed. I will amend by explicitly allocating and freeing the converted string at thecaml_gc_message callsites (and do not worry about freeing in thecaml_fatal_error_arg calls).

Just about to review this too -@gasche, please could we hold off beta2 until a decision is taken on this one. The code page setting part of this GPR I would very much like to be in (I'm preparing a larger post on the Unicode alterations for Windows)

please could we hold off beta2 until a decision is taken on this one

sure

(I have to discuss it with Damien first but I'm thinking of trying a "beta early, beta often" approach this time, which means that I could not wait and just do another beta when you ask for one.)

What's the current behavior and how bad it is? If it's only about displaying garbage for non-ASCII characters in filenames in case of runtime debug messages or fatal errors, this does not sound a critical bug, and it might be better to avoid any risk of regression or rushed solution for 4.06.

The current behavior is that it will either display garbage or even nothing at all (depending on Console code page, etc) for non-ASCII characters in those places where%S is used (runtime debug messages). ASCII characters should come out fine.

TheSetConsoleOutputCP part is about displaying anything at all - without it, you get raw UTF-8 which from a PR (by which I mean "Public Relations") perspective makes it look like we're not properly supporting Unicode. The code to enable it is correctly guarded by theWINDOWS_UNICODE, so legacy mode can continue "unaffected". I can imagine we may end up getting various reports along the lines of "OCaml isn't displaying foreign characters correctly" which get the response "Please run chcp 65001" otherwise!

Note that runningSetConsoleOutputCP affects your process only - the console returns to whichever codepage the user had previously selected when your process terminates.

Note that running SetConsoleOutputCP affects your process only

What about the scenario where the OCaml runtime is embedded in a host application? Could this negatively affect outputs of this application?

Compiled in legacy mode, no difference at all; compiled with WINDOWS_UNICODE=1, the Console would start correctly displaying UTF-8 sequences as the characters they represent - such an application would definitely have been displaying garbage before. It definitely only affects the console, not any file translations.

(it also doesn't affect input processing, which is SetConsoleCP)

I pushed one commit changing to explicitly allocation/deallocation around the fewcaml_gc_message calls. Altogether it is much simpler and we do not even have to introduce a new function.

@dra27:

• I will add some comments to theSetConsoleOutputCP calls.
• What about input? Should we also callSetConsoleCP?
• What is the point of checking whether a Unicode font is available? To print a message to the user? Or simply to expose this to the OCaml programmer?

I have the feeling that we are converging on the matter of how printf stuff should handle the string conversion, but that the ConsoleCP stuff requires another level of expertise and is somewhat orthogonal (although in the end we need to have both for proper Unicode printing, if I understand correctly).@nojb, you may consider splitting the PR in two parts -- but feel free to keep only one if that's easier for you.

@nojb - cosmetically, it would be that we only tell the console to switch to UTF-8 output if it can actually have a chance of displaying the symbols. It would certainly be useful to display that to the user. Cosmetically, setting CP_UTF8 when a truetype font is not selected for the console should cause no change in the output (you just get garbage UTF-8). It certainly doesn'thave to be done in this PR at all, therefore.

@gasche - SetConsoleCP only affects input, we get proper Unicode output on the console without doing that.

I don'tfully understand exactly how these things work with non-Latin keyboards but, for example, if on my Windows box I add Russian keyboard layout, switch the code page to 1250 and have a simple C program which callsgets and displays the output and press the letters ASD on my QWERTY keyboard, then the C program gets three ??? characters. If I alter the program to run SetConsoleCP(CP_UTF8) and repeat the experiment, thengets returns an empty string.

That experiment is enough to suggest that we shouldn't change the way input is being handled - SetConsoleOutputCP is only ever going to affect what is displayed to the user, it's not going (potentially) to change the semantic behaviour of programs.

@alainfrisch Re embedded runtime situation, my understanding is as follows:

• The call toSetConsoleOutputCP fixes the way the bytes output by your process (be it OCaml code or some other code) get interpreted by the Console, which is natively Unicode. So if you are emitting bytes (say, by using C'sprintf), you have to make sure these bytes have the right encoding. But if you are using C# or other .NET things which work with Unicode strings to write to the Console then no translation is actually necessary and things should just work.

• The call toSetConsoleOutputCP will not have any effect whatsoever if standard output/error is redirected (in which they are not really going to the Console but to a file, another process, etc...)

@dra27 do you agree?

@nojb - yes, that's my understanding of it.

@dra27 I added a comment to theSetConsoleOutputCP calls. I would prefer to leave the other bits (figuring out if the font supports Unicode and possibly exposing this inSys) for later.

I think we addressed the other points as well, so in principle I consider this PR ready to be merged.

Would you rebase the PR to remove some back-and-forth, but preferably keep the console-setting and %S-business in separate commits?

@dra27: do you approve the current state? Do you think we should cherry-pick in 4.06?

Rebased.

I approve the first and third commits :-)

I approve the second commit :-) Yes, I think we should cherry-pick this to 4.06 - to a certain extent, it's all part of the "big picture" of Windows Unicode support.

Still investigating some issues (especially aroundMPR#6925) with regard to theSetConsoleOutputCP call.

@nojb I would be able to merge the %S change in 4.06 quickly if you send a separate PR (I'll let@dra27 deal with the console business). We need it at some point anyway: that the ARCHCHARNAT must not be released.

OK, for simplicity I will simply remove commit 2 from this PR, and we can open a new PR for it as needed.

OK, done (and renamed PR for accuracy).

Merged in trunk and cherry-picked in 4.06 (d8e0921 ). (There was really no other choice than cherry-picking as this commit removes an API that we don't want to release.)