Translate OCAML_STDLIB_DIR correctly on Windows#1448
Translate OCAML_STDLIB_DIR correctly on Windows#1448damiendoligez merged 1 commit intoocaml:trunkfrom
Conversation
|
At first I thought that you assumed an environment variable to be unicode, which I found very confusing. But this is actually a macro defined by passing a Then a natural question is: would it make more sense to do this consistently for C macros? As far as I can see, the only other macro in that category is |
|
@dra27 can you break the long line adding the definition of OCAML_STDLIB
to CFLAGS in byterun/Makefile, please?
|
xavierleroy
left a comment
There was a problem hiding this comment.
Impressive scripting kung-fu, as usual.
I agree iconv can be assumed to be installed, but I'm worried about the JAVA encoding. On my Ubuntu 16.04 machine, iconv knows more than 1000 encodings but not any encoding named JAVA.
|
Post-scriptum: the and in Ubuntu we have |
|
From a quick perusal of the two repos, GNU libiconv has had -t JAVA since version 0.3 (1999 - and the oldest commit available in its git repo). AFAICT (I don't understand the politics/history for why there are two), glibc has its own entirely independent implementation of iconv which appears not to have either C99 or Java "encodings". Cygwin relies (and always will rely) on newlib, not glibc, so I think this should be fine. If that changed, |
74f2b1e to
6267424
Compare
|
(no opinion on RUNTIME_NAME?) |
|
@gasche - sorry, I forgot to look at The only time this could benefit is if we wanted to have non-ASCII characters in the name of the runtime, which would be unlikely - if my understanding of headernt.c is fully correct, passing a full path in |
6267424 to
ddaeb48
Compare
|
That's very clear, thanks for the explanation. I think that my consistency-itch could be resolved by explaining it away: you could have a comment at the place where OCAML_STDLIB_DIR is defined, or at the place where it is used, or both, to note that it is passed as a L-string but that other C configuration variables may not. |
|
(While you are at it, there could be a comment around the |
The build generates invalid code in byterun/dynlink.c if LIBDIR contains non-1252 characters. Use iconv to translate LIBDIR to JAVA which, unlike C99 will produce only \u sequences with no \U sequences. The \u is then translated to \x which is the only form recognised pre-Visual Studio 2013.
ddaeb48 to
0739677
Compare
|
That's a good point - I tend to err too much towards commit messages, rather than comments. I've put a comment at the definition of |
|
David Allsopp (2017/10/26 09:06 +0000):
That's a good point - I tend to err too much towards commit messages,
rather than comments.
My way of deciding what should go where is to use commit messages to
document *changes*, while comments are used to document *how* something
works, independently of when it has been changed to work that way.
|
The build generates invalid code in byterun/dynlink.c if LIBDIR contains non-1252 characters. Use iconv to translate LIBDIR to JAVA which, unlike C99 will produce only \u sequences with no \U sequences. The \u is then translated to \x which is the only form recognised pre-Visual Studio 2013. (cherry picked from commit 3b434eb)
|
Cherry-picked in 4.06 : 7f21a3f. |
See #1444.
Certainly for trunk. I agree with @gasche that describing this as build system change was a bit of a stretch. What is eliminated in byterun/dynlink.c is the copying of the string constant supplied by the build system, because the build system is altered to pass the string as UTF-8 in the first place.
The change eliminates a compiler warning emitted by cl if OCaml is compiled in a directory which includes UTF-8 sequences.