Aid compatibility with older Microsoft C compilers

[dra27]

In other projects, it is useful to be able to tell GCC to run with --std=c89 in order to write C code which is compatible with Microsoft's C compiler, but this disables the inline keyword, as that's officially only part of C99.

This GPR proposes altering the headers to use GCC's __inline__ keyword, which is always available in GCC, and introduces a test in configure which adds a definition for __inline__ if the C compiler does not support it.

I think that this "trick" using --std=c89 to force compatibility with MSVC is a safe thing to do, but I'd very much appreciate input from someone who really gets the differences in semantics between -fgnu89-inline (which is also enabled by using --std=c89) and C99 inline.

[oandrieu]

I'm confused, why don't you simply compile you C code with MSVC to check that it's compatible ?

[dra27]

I'll look into what I've done incorrectly with the msvc32 port later

@oandrieu - that forces me or anyone else to be developing my code using msvc or to correct later on (in CI, whatever) after I've fixed other bugs vs having GCC tell me about accidentally C89+isms at the same as my actual bugs. It's also a problem if:

1. You're using mingw64 because it's convenient to have fast access on the command line to both a 32 and 64-bit C compiler
2. You're using Visual Studio 2015 or later
3. You're developing on Linux and don't have msvc but you do have mingw...

[dra27]

msvc32 should be fixed - a little more extra care was needed to ensure that static inline works for building OCaml itself. This has the nice effect that the Caml headers no longer magically makes inline work on MSVC (because the headers use __inline__ always) - see extra code in config/m-nt.h.

@stedolan, in addition to helpfully confirming that there shouldn't be semantic worry because inline is always used in the context of static inline also made the point that because any build system compiling this way must have code somewhere to pass -std=c89 to gcc, one can easily add -D inline=__inline__ at the same time. That said, I think that the side-effect here of not ever redefining the inline keyword on MSVC is a different and more-compelling reason for the change.

[oandrieu]

@dra27 In my experience, the most common incompatibility between old MSVC and C99 is mixing declaration and statements, wouldn't the -Wdeclaration-after-statement GCC warning option be enough ?

Another possibility would be to leave inline in the code and try to detect your special case of gcc-called-with-stdc89, something like this:

#if defined (__GNUC__) && (__STDC_VERSION__ < 199901L)
# define inline __inline__
#endif

[xavierleroy]

The ISO C 1999 standard is old enough to vote in many countries. It makes me sad that we still cannot write the OCaml runtime system in proper C99.

This said, I'm not against the approach proposed here. But I'd rather not use __inline__ as the keyword/macro to use in runtime source code: identifiers and macro names starting with __ are "reserved" (whatever that means) in the C99 standard, and I know from experience that some C standard library headers do weird things with GCC extended keywords, such as defining __attribute__(x) to the empty string.

So, I'd rather have a macro, say Caml_inline, that expands to static inline or static __inline__ or static __inline as appropriate. (Why put static inside the macro? because extern inline or plain inline have bizarre semantics and I'd rather make sure we never run into these.)

Alternatively, @oandrieu's suggestion (#define inline __inline__ when appropriate) makes a lot of sense and is much simpler.

[xavierleroy]

@dra27 do you wish to pursue this approach, esp. the #define inline __inline__ suggestion of @oandrieu? Or should we close?

[dra27]

Sorry, I'd put this on the proverbial backburner! Thanks @oandrieu - I didn't know about that switch and I've now changed my original use case in opam to use that instead of --std=c89.

We should tidy up the #define inline stuff as suggested and I will try to do that in the next 48 hours (and also tweak AppVeyor and ci-build to use -Wdeclaration-after-statement

[xavierleroy]

Any news on this PR? If not we'll take it off the 4.06 milestone, as it is not particularly urgent.

[xavierleroy]

Taking this PR off the 4.06 milestone.

[xavierleroy]

No news for two years. C89 support looks less and less relevant. We should rather encourage the use of modern C standards. I'm taking the liberty to close this PR.

[dra27]

The build failure in #9293 on msvc32 prompted me finally to finish this one off, because it would have prevented it.

PR updated:

• @xavierleroy's suggestion to introduce Caml_inline is done. caml_stat_strdup_to_utf16 was marked inline in runtime/win32.c which is completely incorrect.
• configure now automatically uses -Wdeclaration-after-statement if GCC is in use, which necessitates juggling in a few C files.

Combined, that means we:

• no longer abuse the inline keyword on MSVC
• don't waste AppVeyor time where a developer reasonably forgets to put a variable declaration only at the start of a block

I have not updated the inline keywords in stdlib/headernt.c because that file is MSVC/mingw-w64 specific and I didn't see any value in working out the various versions of __forceinline.

[xavierleroy]

Wouldn't it be nice if MSVC implemented the C99 standard... But I'm afraid we have to carry that cross.

This said, even for C compilers that implement C99, it is better to use static inline instead of inline, and the Caml_inline macro encourages this good practice.

So, this PR is an improvement, and I'm in favor of merging. One suggested change below, but nothing essential.

[xavierleroy]

Since Caml_inline is available on all platforms, I would simply replace INLINE by Caml_inline in this file and delete this #ifdef nonsense.

[dra27]

I wasn't totally sure about that, because inline was only used for GCC, but I guess that could have been dropped when we introduced the C99 requirement. I pushed a commit tidying it up.

[XVilka]

Isn't MSVC 2015 and higher versions implement C99? We (radare2) dropped older MSVC compilers precisely because of this, and enforced C99 usage.

…

On Thu, Feb 13, 2020, 02:12 Xavier Leroy ***@***.***> wrote: ***@***.**** approved this pull request. Wouldn't it be nice if MSVC implemented the C99 standard... But I'm afraid we have to carry that cross. This said, even for C compilers that implement C99, it is better to use static inline instead of inline, and the Caml_inline macro encourages this good practice. So, this PR is an improvement, and I'm in favor of merging. One suggested change below, but nothing essential. ------------------------------ In otherlibs/systhreads/st_posix.h <#1176 (comment)>: > #else -#define INLINE +#define INLINE static Since Caml_inline is available on all platforms, I would simply replace INLINE by Caml_inline in this file and delete this #ifdef nonsense. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1176?email_source=notifications&email_token=AABRT7PJYF7CMCHVR5XSGR3RCQ3YZA5CNFSM4DL4NLT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCVI2ENQ#pullrequestreview-357671478>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABRT7JABUEBOKLBDBAHV43RCQ3YZANCNFSM4DL4NLTQ> .

[dra27]

Thanks, @xavierleroy - I'll merge once CI confirms the additional commit.

@XVilka - IIUC, no version of MSVC is C99-compliant. When Microsoft rewrote their C runtime library in C++ to create the Universal CRT, the C99 support was vastly improved, and consequentially some C99 things were added to the compiler, but I think their stock answer has always been that it's a C++ compiler (and I believe the adherence to ISO C++ is at least less awful...)