Fix naming of shared Unicode stubs by nojb · Pull Request #1363 · ocaml/ocaml

nojb · 2017-09-21T11:02:11Z

This PR fixes a naming oversight in #1200. There are three functions used in C code shared between Unix and Windows to support Unicode under Windows which are called

caml_stat_strdup_to_utf16 [converts from OCaml-side to OS-side]
caml_stat_strdup_of_utf16 [converts from OS-side to OCaml-side]
caml_copy_string_of_utf16 [converts from OS-side to OCaml-side]

These names make no sense under Unix (where they are just synonyms for caml_stat_strdup, caml_stat_strdup, and caml_copy_string respectively). Let us rename them to something more neutral. In #1200 @dra27 had suggested using _os as suffix (I lost track of the suggestion back then):

caml_stat_strdup_to_os
caml_stat_strdup_of_os
caml_copy_string_of_os

Opinions ?

While on this topic, it may be worthwhile to revisit the other names introduced by #1200, see here. For example, the name _texecve stands for execve under Unix and _wexecve under Windows. The reason for this naming choice was partly due to it being like this in the original Unicode PR and partly because it follows Windows conventions. But I would be happy to hear any suggestions for improvements.

Also: the changes in #1357 will need to be updated to take into account this PR.

@gasche @alainfrisch @dra27

alainfrisch · 2017-09-21T11:15:10Z

*_to_os seems good to me.

What about execve_os, and char_os instead of wchar_t?

nojb · 2017-09-21T11:19:16Z

execve_os and the like seem fine to me. For the character type we use charnat but it should be renamed char_os for consistency, indeed.

I will wait a little before updating the PR to give more time in case there are other opinions.

alainfrisch · 2017-09-21T11:22:19Z

byterun/startup.c

 #ifdef _WIN32

-static wchar_t * read_section_to_utf16(int fd, struct exec_trailer *trail, char *name)
+static wchar_t * read_section_to_os(int fd, struct exec_trailer *trail, char *name)


That's where I found wchar_t. Should this be charnat (resp. char_os) for consistency?

Ping on this minor point.

Thanks, fixed.

nojb · 2017-09-21T15:02:17Z

Rebased on trunk.

gasche · 2017-09-23T08:12:22Z

I still have some questions about the use of caml_stat_strdup{,of_os,to_os} functions from my newbie perspective:

What are the reasons to use caml_stat_strdup directly instead of caml_stat_strdup_of_os? My guess would be that it is when you really want char * instead of oschar *, but it's not clear to me that it actually happens within the compiler distribution. Furthermore, there remain 25 uses of caml_stat_strdup within otherlibs/unix (git grep "caml_stat_strdup(" otherlibs/unix), versus 11 uses of caml_stat_strdup_to_utf16, can all of these be converted to to_os? (Or is there a good reason for the inconsistency?)
same question with caml_copy_string_of_os: when do we not want to use it?
caml_stat_strdup has a _noexc variant. Would such variants be useful for the new _{of,to}_os functions and other GC-using functions introduced by this patch?
when do we want to use caml_stat_strdup_of_os? Currently there is one use in debugger.c that I don't understand, one use in win32.c, and all other uses are to be passed to a %s format (usually in an error function, so the allocation is never explicitly collected)

nojb · 2017-09-23T08:51:44Z

Thanks for the questions @gasche ! Indeed, simplifications along the lines you mention may be possible.

Basically the intention with the _os-suffixed functions vs the non-suffixed functions was to make it explicit that the argument (or result) was being received from (or sent to) the OS and had a particular encoding. On the other hand, caml_{stat_strdup,copy_string} can be used to copy arbitrary byte buffers, but as you point out this may not actually happen in the compiler distribution.
Another point was to make the diff as small as possible and make it clear that the parts of the unix library that are not shared with Windows remain unmodified.
For caml_stat_strdup_of_os you are right there are not many uses. This function was introduced more as a way to keep the diff small by using existing code that assumed char *, but indeed it could be eliminated by some further refactoring.

All in all I agree that it is very appealing to try to get rid of the _os variations and just have caml_stat_strdup and caml_copy_string. I will experiment along these lines and report the results.

Just to summarize your suggestion is to change the signature of caml_{stat_strdup,copy_string} to

CAMLextern os_char * caml_stat_strdup(const char *s);
CAMLextern value caml_copy_string (os_char const *);

(where os_char is char on Unix and WCHAR on Windows), and use that everywhere.

xavierleroy · 2017-09-23T08:55:10Z

All in all I agree that it is very appealing to try to get rid of the _os variations and just have caml_stat_strdup and caml_copy_string

No way. It's perfectly legitimate to copy bytes as just byte with no OS-specific encoding implied. That the need doesn't show up in today's (Windows version of) the runtime system is an accident.

gasche · 2017-09-23T09:03:07Z

What I think would be useful to have by release time are clear guidelines on how to write portable string-manipulating C code for the OCaml runtime (inside the distribution or in user's bindings), and hopefully to have these guidelines be followed by the code in the compiler distribution.

(Another interesting question is whether we understand the backward-compatibility impact of these guidelines, that is the ability to write code that works under both old and new OCaml versions. I suppose that currently the answer is "wait until the oldest version you want to support has _os functions, and switch only then".)

xavierleroy · 2017-09-23T16:35:00Z

The _os names look OK to me, by the way. Concerning the other question:

While on this topic, it may be worthwhile to revisit the other names introduced by #1200, see here. For example, the name _texecve stands for execve under Unix and _wexecve under Windows.

I'm not a big fan of those function names that start with an underscore, first because they scream "Windows!", second because the ISO C99 standard has a few choice words to say about them (7.1.3):

— All identifiers that begin with an underscore and either an uppercase letter or another
underscore are always reserved for any use.
— All identifiers that begin with an underscore are always reserved for use as identifiers
with file scope in both the ordinary and tag name spaces.

The Windows names such as _wexecve fit this spec, mind you. Still, so many restrictions make me nervous. So, I welcome alternative ideas for names, although I have none to offer at the moment.

nojb · 2017-09-25T11:06:53Z

What about using the _os suffix for all such symbols?

char_os for the character type char / WCHAR,
execve_os for execve/_wexecve, and so on, and
caml_stat_strdup_{of,to}_os for caml_stat_strdup, etc.

gasche · 2017-09-25T12:07:38Z

It seems fine to me.

gasche

I approve of this change. Would a third person have a look, and merge if they also agree?

gasche · 2017-09-26T07:34:28Z

On the other hand, I believe that this needs to be rebased on top of the just-merged #1357 before merging.

alainfrisch · 2017-09-26T07:45:58Z

It's hard to do a useful review of such mostly mechanical renaming, but this all looks good to me, and @xavierleroy validated the terminology, so I think this is good to merge after rebasing. (There is also just one pending minor comment.)

nojb · 2017-09-26T08:56:33Z

Incidentally the Travis failures seem to be unrelated to the Unicode stuff, having to do with the symbol caml_atom_table.

gasche · 2017-09-27T09:40:19Z

Note: this will have to be rebased again when #1362 is merged.

The caml_atom_table failure is a separate issue that already occurs in trunk, so we will handle it in another place. It should not block merging this PR.

dra27 · 2017-09-27T09:55:12Z

LGTM too and ready to rebase now that #1362 is merged.

nojb · 2017-09-27T10:00:32Z

Rebased on trunk.

nojb · 2017-09-27T11:58:02Z

Anyone wants to restart Travis, or merge directly ?

gasche · 2017-09-27T12:39:10Z

I restarted the CI build. We have long been waiting to see the greens!

gasche · 2017-09-27T13:00:03Z

@nojb: could you maybe add this PR number to the Changes entry about the Windows runtime? It would be nice if someone following the PRs could get the whole picture -- especially given that currently those discussions and sources are the only source of documentation for the C interface.

nojb · 2017-09-27T13:00:56Z

Sure, makes sense.

nojb · 2017-09-27T13:07:40Z

OK, added all related PRs #1357, #1362, #1363 to Changes.

nojb · 2017-09-27T14:35:13Z

All green! Merge?

nojb · 2017-09-27T14:41:25Z

Thanks!

dra27 · 2017-09-27T14:41:48Z

Huzzah - well done @nojb!

Fix naming of shared Unicode stubs

gasche · 2017-09-27T14:42:53Z

Merged, and cherry-picked in 4.06 ( 6882353 ).

shindere · 2017-09-27T14:55:15Z

So good to be back to a healthy trunk! Incidentally: we may try to do a better job at uniformizing our different CI scripts. For instance, we may want to architecture things so that we can guarranty that some things are tested on Inria's CI, AppVeyor and Travis. We don't do that yet, e.g. the tests are not yet run with ocamlrund on Inria's CI. If nobody has done so before, I'll probably try to have a look to this in not too long.

* Create invitation-to-contribute * Rename invitation-to-contribute to invitation-to-contribute.md * remove kind from title space * added description field * Delete invitation-to-contribute.md * Create invitation-to-contribute.md * fix image link * image for contribute * added calendar dates and Discord link * Update data/news/ocamlorg/invitation-to-contribute.md Co-authored-by: sabine <sabine@users.noreply.github.com> * Update data/news/ocamlorg/invitation-to-contribute.md Co-authored-by: sabine <sabine@users.noreply.github.com> --------- Co-authored-by: sabine <sabine@users.noreply.github.com>

alainfrisch reviewed Sep 21, 2017

View reviewed changes

nojb force-pushed the fix_1200_naming branch from b24c716 to c18897b Compare September 21, 2017 11:22

shindere mentioned this pull request Sep 21, 2017

Fix ocamltest / Windows / Unicode issues #1357

Merged

nojb changed the base branch from 4.06 to trunk September 21, 2017 15:00

nojb force-pushed the fix_1200_naming branch from c18897b to 7060fda Compare September 21, 2017 15:01

gasche mentioned this pull request Sep 21, 2017

Document the Unicode mode for Windows #1360

Merged

damiendoligez added this to the 4.06.0 milestone Sep 22, 2017

gasche approved these changes Sep 26, 2017

View reviewed changes

dra27 mentioned this pull request Sep 26, 2017

Fix headernt.c & -output-obj Unicode support #1362

Merged

nojb force-pushed the fix_1200_naming branch from bd64432 to aa96a54 Compare September 26, 2017 08:25

Rename: use _os suffix

799f36d

nojb force-pushed the fix_1200_naming branch from aa96a54 to 799f36d Compare September 27, 2017 10:00

Update Changes

8031d0a

gasche merged commit 050ceef into ocaml:trunk Sep 27, 2017

gasche added a commit that referenced this pull request Sep 27, 2017

Merge pull request #1363 from nojb/fix_1200_naming

6882353

Fix naming of shared Unicode stubs

Conversation

nojb commented Sep 21, 2017

Uh oh!

alainfrisch commented Sep 21, 2017

Uh oh!

nojb commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alainfrisch Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

alainfrisch Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

nojb Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

gasche commented Sep 23, 2017

Uh oh!

nojb commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xavierleroy commented Sep 23, 2017

Uh oh!

gasche commented Sep 23, 2017

Uh oh!

xavierleroy commented Sep 23, 2017

Uh oh!

nojb commented Sep 25, 2017

Uh oh!

gasche commented Sep 25, 2017

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

gasche commented Sep 26, 2017

Uh oh!

alainfrisch commented Sep 26, 2017

Uh oh!

nojb commented Sep 26, 2017

Uh oh!

gasche commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Sep 27, 2017

Uh oh!

nojb commented Sep 27, 2017

Uh oh!

nojb commented Sep 27, 2017

Uh oh!

gasche commented Sep 27, 2017

Uh oh!

gasche commented Sep 27, 2017

Uh oh!

nojb commented Sep 27, 2017

Uh oh!

nojb commented Sep 27, 2017

Uh oh!

nojb commented Sep 27, 2017

Uh oh!

nojb commented Sep 27, 2017

Uh oh!

dra27 commented Sep 27, 2017

Uh oh!

gasche commented Sep 27, 2017

Uh oh!

shindere commented Sep 27, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

nojb commented Sep 21, 2017 •

edited

Loading

alainfrisch Sep 21, 2017 •

edited

Loading

nojb commented Sep 23, 2017 •

edited

Loading

gasche commented Sep 27, 2017 •

edited

Loading