I don't think this should be merged as-is since it would change many things for users of the compiler.

The fact is that today, all libraries are built with $bytecccompopts and dropping these options can impact all of them In particular, options such as -fno-strict-aliasing and -fwrapv can have an impact on the code that is included in ocaml headers with e.g. macros (I haven't checked but it /is/ a possibility). Moreover, flags such as -O2 and -Wall are useful to provide to the build of libraries.

I concur it is better to have compiler-specific options and the current state has bitten me too in the past but we also probably want to keep default ones for the libraries too. Even for C stubs we probably don't want to authors to have to think too much about C and its constraints. Lastly, there can be ABI issues and for instance config/Makefile.mingw{,w64} use {BYTE,NATIVE}CCCOMPOPTS to pass -mms-bitfields which has an impact on ABI; no OCaml library author should have to think about this or everything will break.

(NB: -mms-bitfields has been the default for some time for mingw ports but it doesn't harm and it's more work to make it conditional on the compiler versions that really need it)

I would probably introduce a new COMPILERCOMPOPTS or something like that in order to hold the compiler-specific flags such as -DCAML_NAME_SPACE.

Adding the options in another variable or in BYTECC and NATIVECC should lead to the same result. As such I have no opinion on which one should be done over the other.

As for -O2 and -Wall, my current concern is that the various build systems for ocaml and the compiler itself haven't really shined when it comes to passing options to the C compiler. I would prefer that this is improved before the set of flags changes.

The PR has been rebased and updated to take your comments into account,
@adrien-n.

Most options are passed to C compilers.
The only one that are not passed any longer are the warning-related ones.
This has been decided in agreement with @damiendoligez.

@alainfrisch: the SHAREDCCCOMPOPTS variable contained the -O option on the
MinGW port and the -Ox option on the msvc port. This PR removes these
options from the variable. Could you please confirm this is okay?

SHAREDCCCOMPOPTS variable contained the -O option on the MinGW port and the -Ox option on the msvc port

I guess this is ok, but this will change (silently) how C user code is compiled, no?

Alain Frisch (2016/11/15 05:44 -0800):

SHAREDCCCOMPOPTS variable contained the -O option on the MinGW port and the -Ox option on the msvc port

I guess this is ok, but this will change (silently) how C user code is
compiled, no?

On the MinGW port the -O option is already present in the BYTECC
variable, thus there should be no change at all, except that it will be
passed only once instead of twice.

For the msvc port, there is already a -O2 present in BYTECC, so without
this patch cl is called with -O2 -Ox, I don't know which one takes
precedence in this case. With the patch applied -O2 remains. If -Ox is
prefered I can update the PR accordingly, i.e. replace -O2 by -Ox in
BYTECC.

If -Ox is prefered

@dra27 Do you have an opinion?

It should possibly be explicitly highlighted in Changes but, realistically, this would only be being used for C stubs in user code, surely? So I can't think optimisation levels are likely to be that critical (both in terms of code generated and code emitted) - so why not go with -Ox?

For reference, using -O2 instead of just -O for gcc/clang was discussed and decided in #226. There was no mention of MSVC in this discussion. O2 in MSVC actually appeared earlier than that, in @damiendoligez's commit 20c278b which references MPR#6590, in which Damien carefully considered the issue and selected -O2 -Gy- (is that a thing?) going forward.

I have no competence to review the PR in general, but on the specific choice of O2/Ox, MPR#6590 seems to suggest that -Ox is now deprecated, so -O2 is probably the better default.

-Ox is not deprecated, but there advice to choose between -O1 or -O2 (see MSDN) which shows I should have checked MSDN before commenting previously (sorry)!

I guess the key thing is whether the testsuite has been fixed subsequently, or whether the -O2 -Ox in BYTECC has masked the issue since 4.02. Given that both -O2 and -Ox are aliases for other flags, I expect this has been fine as presumably it's come out as

• -O2
• -Gy- (disabling the -Gy in -O2, I think)
• -Ox (which adds no new options not already present)

Given that Appveyor's passing (and we've released two versions of OCaml since based on this), it would seem that -O2 -Gy- is working fine?

Bear in mind when looking for comparisons with gcc that -O1 and -O2 are just different strategies - it's not strictly implying that -O2 is more aggressive in the way that gcc's -O option works (where higher n does imply more attempts to optimise).

Looks good to me but note I can't say much about the MSVC part. My only question is whether we have (roughly) equivalent warning flags between MSVC and others (others have -Wall -Wno-unused, MSVC has nothing) but that's very minor.

@adrien-n - indeed, the issue of warnings in MSVC also came up discussions around #912. The equivalent I think is -Wall -wd4100 -wd4101 -wd4102... I'm just running a few tests.

Wowzers - I thought that might happen! There's a reason that -Wall is not used in the MSVC ports 😄. There's quite a list of warnings which would need to be gone through and turned off!

David Allsopp (2016/11/15 10:05 -0800):

Wowzers - I thought that might happen! There's a reason that -Wall is
not used in the MSVC ports 😄. There's quite a list of warnings
which would need to be gone through and turned off!

Thanks a lot for having tested, David!

Going through those warnings, trying to fix them etc. is definitely on
my list for the very short term future, hopefully still this week.

And many thanks for the warnings you have suggested, will start from
there.

Will also use your suggestion for optimization options.

You mentionned testsuite failures. What were you refering to, exactly?

The Mantis report mentioned testsuite failures - but I think these were fixed by the addition of -Gy-. Note that Microsoft C includes a lot of warnings which are in fact linting issues, so not necessarily "to be fixed". It looks to me like aspiring to -W3 with a few -Wdn is the best result. Lots of the warnings look very opinionated or deal with corner cases that don't matter (e.g. complaining that unary minus is applied to an unsigned value in the Tag_val macro!)

adrien-n (2016/11/15 09:10 -0800):

Looks good to me but note I can't say much about the MSVC part. My
only question is whether we have (roughly) equivalent warning flags
between MSVC and others (others have -Wall -Wno-unused, MSVC has
nothing) but that's very minor.

So just to make sure everything is written out clearly: no, at the
moment the msvc build does not have equivalent warnings set up. But this
is supposed to come very soon.

Given that there seems to be a consensus, I went ahead and merged. Thanks!

Moving towards using -W3 in the msvc ports is related to MPR#5079