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

Always define WINDOWS_UNICODE internally #10103

Closed
wants to merge 1 commit into from

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Dec 29, 2020

Rather than just defining WINDOWS_UNICODE for the Windows builds, define it for all builds (it's just always 0 on Unix). Split off from #9284.

@xavierleroy
Copy link
Contributor

I can see many a Linux or macOS users surprised by seeing WINDOWS_UNICODE on the command line when doing ocamlopt -verbose or watching build logs. What is the motivation for this change?

@dra27
Copy link
Member Author

dra27 commented Dec 31, 2020

They won't see that - this only goes to OC_CPPFLAGS (calling the C compiler during the build), not OCAMLC_CFLAGS/OCAMLOPT_CFLAGS (ocamlc/ocamlopt calling the C compiler)

#9284 tests if WINDOWS_UNICODE is 1 in the pre-processor - the behaviour will be correct if it's not defined, but it seemed better (to me) to have it always defined.

@xavierleroy
Copy link
Contributor

They won't see that - this only goes to OC_CPPFLAGS (calling the C compiler during the build), not OCAMLC_CFLAGS/OCAMLOPT_CFLAGS (ocamlc/ocamlopt calling the C compiler)

OK, but it will still show up in the build logs for OCaml itself.

As you mention, #if WINDOWS_UNICODE will evaluate to false if WINDOWS_UNICODE is undefined, and nothing bad will happen. If you find this too obscure, #if defined(WINDOWS_UNICODE) && WINDOWS_UNICODE will do the job. None of this warrants a change to configure and even longer command lines when building OCaml.

@dra27
Copy link
Member Author

dra27 commented Jan 3, 2021

I originally didn't think I'd get away with relying on the defined behaviour for undefined variables 🙂

@dra27 dra27 closed this Jan 3, 2021
@dra27 dra27 deleted the unicode-config branch July 6, 2021 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants