Skip to content

Windows Unicode support for ocamlyacc#8621

Merged
dra27 merged 6 commits intoocaml:trunkfrom
dra27:win_unicode-ocamlyacc
Sep 27, 2019
Merged

Windows Unicode support for ocamlyacc#8621
dra27 merged 6 commits intoocaml:trunkfrom
dra27:win_unicode-ocamlyacc

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Apr 16, 2019

This is part of #1200 which didn't make the merge window for 4.06.0.

The motivation behind this is the elimination of MKEXE_ANSI, the presence of which is hurting some work I'm trying to do both extending the FlexDLL bootstrap to the Cygwin ports and extending configure's detection support for FlexDLL/flexlink.

This PR does not alter the way .mly files are interpreted, rather it permits ocamlyacc to receive names on the command line which include extended characters. For the most part this is irrelevant, since most resulting names won't be valid module names.

stdlib.h is included in defs.h
Using an extern disables error checking of the parameters in "modern"
compilers.
This deals with the command line processing only (i.e. filenames) -
ocamlyacc continues to process .mly files as before.
@dra27 dra27 force-pushed the win_unicode-ocamlyacc branch from fce7eea to c066fb1 Compare April 18, 2019 13:11
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Apr 18, 2019

It was apparently a mistake to try to combine this PR with also eliminating MKEXE_ANSI from the build system. Assuming this finally builds, I ask for this to be merged first, and then a PR will follow with the additional work for removing MKEXE_ANSI completely.

@dra27 dra27 requested a review from nojb April 18, 2019 13:14
@dra27 dra27 force-pushed the win_unicode-ocamlyacc branch from c066fb1 to 3f6f6f1 Compare April 19, 2019 20:36
@dra27 dra27 mentioned this pull request May 1, 2019
Copy link
Copy Markdown
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a first pass. Two remarks:

  1. It doesn't seem right to use the runtime headers when compiling ocamlyacc. It makes the code less self-contained and I wonder how much work are we really saving. As far as I can tell, all we need is a bunch of _os macros which could easily be added to defs.h.

  2. You support compiling both in Unicode and ANSI mode by switching windows_unicode_enabled. Is there any reason for this? If not, I would just compile for Unicode unconditionally.

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Jun 10, 2019

@nojb - ocamlyacc was already using the runtime headers, except that before there was duplication between what was in ocamlyacc and what was in the runtime (with a non-fatal error where mingw was concerned). I don't think there's any particular reason for ocamlyacc to be kept self-contained, it's just that it happens not to be "original"?

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Jun 10, 2019

I don't have a strong opinion either way on windows_unicode_enabled - it was just that in having to lift that function out of the runtime (since it uses OCaml's memory management functions), I either had to hard-code the value or just duplicate the variable. The only reason for doing it this way is that you then don't end up with an ocamlyacc which does recognise UTF8 alongside a compiler which does not... but it's a largely irrelevant distinction. Does it bother you enough to require changing?

@dra27 dra27 force-pushed the win_unicode-ocamlyacc branch from 3f6f6f1 to 7482ff4 Compare June 10, 2019 12:54
Copy link
Copy Markdown
Member

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me, you may want to get a nod from @nojb before merging.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Sep 19, 2019

Oups! Very sorry I completely forgot about this one. Yes, I had a few slight questions/remarks about the PR but was convinced by @dra27 answers, so I'm approving as well :)

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Sep 27, 2019

I'll merge once CI gives the nod.

Contrary to @XVilka's excitement in #8797, this PR adds very little of interest for the Windows platform but, as noted above, it does allow me to dust off the revisions to autoconf for FlexDLL bootstrapping, so thanks for the approval, @nojb!

@dra27 dra27 merged commit 150d476 into ocaml:trunk Sep 27, 2019
@dra27 dra27 deleted the win_unicode-ocamlyacc branch September 27, 2019 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants