Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove compile warnings under windows #165

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
5 participants
@bschommer
Copy link
Contributor

commented Apr 14, 2015

This patch removes some compile warnings when compiling Ocaml with the Visual Studio under windows by adding:

  • Define guards in the win32unix/unixsupport.c as in the other version
  • Define guards in byterun/floats.c around isnan and isfinite

Also changed the definition of the Noreturn Macro in misc.h so that it also works with a Visual Studio version newer then 2008.

@bactrian bactrian force-pushed the ocaml:trunk branch from da1f9c2 to 98b4109 Apr 18, 2015

@gasche

This comment has been minimized.

Copy link
Member

commented May 6, 2015

It seems there is an extraneous patch wrongly included in the PR (there was a small hiccup of the SVN/github mirror when this patch of Jeremy Yallop was merged, and I wonder whether this explains why I don't remember having seen your PR before). Would you rebase it against the current trunk and git push --force into your own branch to refresh the PR?

bschommer added some commits Apr 14, 2015

Changed the definition of the Noreturn macro to also work with Visual…
… Studio Version >= 2008 using the __declspec(noreturn).
@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2015

I removed the extraneous patch.

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2015

The potential compile warnings from the CAMLparam and CAMLlocal macros could also be removed when the initialization of the caml__dummy_##x variable

  CAMLunused int caml__dummy_##x = ( \
    (caml__roots_##x.next = caml_local_roots), \
    (caml_local_roots = &caml__roots_##x), \
    (caml__roots_##x.nitems = 1), \
    (caml__roots_##x.ntables = 1), \
    (caml__roots_##x.tables [0] = &x), \
    0)

is replaced by

  CAMLunused int caml__dummy_##x = ( \
    (caml__roots_##x.next = caml_local_roots), \
    (caml_local_roots = &caml__roots_##x), \
    (caml__roots_##x.nitems = 1), \
    (caml__roots_##x.ntables = 1), \
    (caml__roots_##x.tables [0] = &x), \
    ((void)caml__dummy_##x,
    0)

Which is valid in C. However I'm not sure if there are any C compiler that don't support this. I just tried it with gcc and clang and it seems okay.

@@ -170,45 +170,118 @@ void win32_maperr(DWORD errcode)
}

/* Windows socket errors */

#ifndef EWOULDBLOCK

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy May 7, 2015

Contributor

What happens if EWOULDBLOCK is already defined but is not equal to -WSAEWOULDBLOCK ? Would that break the reporting of errors as a Unix_error exception?

This comment has been minimized.

Copy link
@bschommer

bschommer May 7, 2015

Author Contributor

You are right, erno defines them to different values. I think the better solution would be to just replace the uses of the macro in this file by the corresponding value.

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2015

The last commit avoids the problem which occur when the error values are defined differently by just undefining them before redefining the values.

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented on 81dfa68 May 7, 2015

The define from trapsp to caml_trapsp in compatibility.h changed the trapsp to caml_trapsp and used the uninitialized variable to initialize itself.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented May 7, 2015

The define from trapsp to caml_trapsp in compatibility.h changed the trapsp to caml_trapsp and used the uninitialized variable to initialize itself.

Yes, I've seen this one (or another instance) recently (and I thought I'd fixed it) where gcc didn't warn, but MSVC complained. This makes me suspicious of your solution for caml__dummy_##x because it's basically the same thing: a use of the variable in its own initializer.

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2015

I just tested the caml__dummy##x solution and it did not help. MSVC still complained about unused variables.

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2015

I added some MSVC specific pragmas which disable the warning C4189 (unused local variable) for the caml__dummy##x variable.

@@ -192,35 +192,48 @@ CAMLextern struct caml__roots_block *caml_local_roots; /* defined in roots.c */


#if defined(__GNUC__) && (__GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 7))
#define CAMLunused __attribute__ ((unused))
#define CAMLunused_start __attribute__ ((unused))

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez May 13, 2015

Member

Why did you rename this macro?

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2015

Since the pragma solution for windows requires a push before and a pop after I thought it would sense to rename the macro which adds the push into start and the one that ats the pop into end but I do not insist on this renaming.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented May 13, 2015

Oh that makes sense. I'd overlooked the "end" part, sorry.

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2015

I updated the my pull request to get a working travis build again, the problems came from other commits.
Concerning the new names for the CAMLunused macro: Does somebody rely on them being in the header since otherwise I could add the old one back for compatibility reasons.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 26, 2015

I merged the patch series in trunk (except for the trapsp one that is superseded by af15f03 ). Thanks for the patches!

@gasche gasche closed this Jul 26, 2015

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2015

This patch changes the way "Noreturn" should be used. This breaks third-party stubs, for instance sqlite3-ocaml. Perhaps we should think of a more backward compatible approach, keeping the old definition of Noreturn (ignored under MSVC) and providing a new macro with a different onem to be used in prefix position.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 28, 2015

We could provide Noreturn_start and Noreturn_end, and have CAMLunused a compatibility alias of CAMLunused_start and Noreturn a compatibility alias of Noreturn_end. Isn't the start/end pair a bit heavy, though?

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2015

You could also just add one Noreturn_internal or so for the header of the standard library and keep the old definition for the backward compatibility.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 28, 2015

But it means that third-party stubs using the (non-internal) standard headers have no way to fix their own MSVC unused warnings.

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2015

Yes you are right. But I would rename the new macro to CAMLnoreturn since C11 introduces a noreturn macro.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 28, 2015

Would you propose a new patch(set) for whichever approach you think is best?

@gasche gasche reopened this Jul 28, 2015

@bschommer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2015

No problem.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 2, 2015

@alainfrisch Noreturn is now back to its previous semantics, restoring compatibility. Thanks for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.