check _WIN32 rather than WIN32 #22

Merged
merged 2 commits into from Feb 12, 2016

Conversation

Projects
None yet
3 participants
@kevinushey
Contributor

kevinushey commented Feb 11, 2016

For new Rtools compatibility.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Feb 12, 2016

Owner

Thanks mich!

Explain to me again how this did not break before? Because Rtools used to support both?

Owner

eddelbuettel commented Feb 12, 2016

Thanks mich!

Explain to me again how this did not break before? Because Rtools used to support both?

eddelbuettel added a commit that referenced this pull request Feb 12, 2016

Merge pull request #22 from kevinushey/patch-1
check _WIN32 rather than WIN32

@eddelbuettel eddelbuettel merged commit 37acc63 into eddelbuettel:master Feb 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonathon-love

This comment has been minimized.

Show comment
Hide comment
@jonathon-love

jonathon-love Feb 12, 2016

Contributor

hi,

this PR isn't enough, it still won't compile with -std=c++11. let me investigate.

looks to me like WIN32 was and is a legit macro, but under -std=c++11 this is no longer the case.

Contributor

jonathon-love commented Feb 12, 2016

hi,

this PR isn't enough, it still won't compile with -std=c++11. let me investigate.

looks to me like WIN32 was and is a legit macro, but under -std=c++11 this is no longer the case.

@jonathon-love

This comment has been minimized.

Show comment
Hide comment
@jonathon-love

jonathon-love Feb 12, 2016

Contributor

oh, ok, it looks like the issue is that the R headers (in 3.2.3 at least) are still using #ifdef WIN32

line 77 here:
http://docs.rexamine.com/R-devel/RStartup_8h_source.html

are you interested in following this up @eddelbuettel ? brian ripley always hurts my feelings.

Contributor

jonathon-love commented Feb 12, 2016

oh, ok, it looks like the issue is that the R headers (in 3.2.3 at least) are still using #ifdef WIN32

line 77 here:
http://docs.rexamine.com/R-devel/RStartup_8h_source.html

are you interested in following this up @eddelbuettel ? brian ripley always hurts my feelings.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Feb 12, 2016

Owner

Hm, why are tossing around URLs to a 3rd party site on friggin' GitHub when there is wch's well known mirror?

As you are the one troubled by this, could you possibly try something with all of

  • r-release
  • r-devel
  • whatever R you can use with Rtools using 4.9.* ?

Anything else we are missing, @kevinushey ?

Otherwise I may just revert and close this as Rtools with 4.9.* is not yet operational and we do releases here for CRAN. Which passed. And still pass.

That said, we may as well plan for a world with a real C++ compiler so your probing is appreciated, @jeroenooms !

Owner

eddelbuettel commented Feb 12, 2016

Hm, why are tossing around URLs to a 3rd party site on friggin' GitHub when there is wch's well known mirror?

As you are the one troubled by this, could you possibly try something with all of

  • r-release
  • r-devel
  • whatever R you can use with Rtools using 4.9.* ?

Anything else we are missing, @kevinushey ?

Otherwise I may just revert and close this as Rtools with 4.9.* is not yet operational and we do releases here for CRAN. Which passed. And still pass.

That said, we may as well plan for a world with a real C++ compiler so your probing is appreciated, @jeroenooms !

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Feb 12, 2016

Contributor

I don't think there's anything else that needs to be done on the RInside side. You could probably just add something like:

#ifdef _WIN32
# define WIN32
#endif

before including the R headers. That said, I think that _WIN32 is the 'canonical' way of declaring Windows, although I forget the nuances of when / if / who should define WIN32 vs. _WIN32 and so on...

Contributor

kevinushey commented Feb 12, 2016

I don't think there's anything else that needs to be done on the RInside side. You could probably just add something like:

#ifdef _WIN32
# define WIN32
#endif

before including the R headers. That said, I think that _WIN32 is the 'canonical' way of declaring Windows, although I forget the nuances of when / if / who should define WIN32 vs. _WIN32 and so on...

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Feb 12, 2016

Owner

I am pretty I went over this once in the past. This site was always a good resource, and it has _WIN32 as the canonical one. The R 3.2.3 source and the R-devel source appear to have both.

Owner

eddelbuettel commented Feb 12, 2016

I am pretty I went over this once in the past. This site was always a good resource, and it has _WIN32 as the canonical one. The R 3.2.3 source and the R-devel source appear to have both.

@jonathon-love

This comment has been minimized.

Show comment
Hide comment
@jonathon-love

jonathon-love Feb 12, 2016

Contributor

Hm, why are tossing around URLs to a 3rd party site on friggin' GitHub when there is wch's well known mirror?

apparently news hadn't made it to my search engine :P

As you are the one troubled by this, could you possibly try something with all of

looking here, there's still the #ifdef Win32

https://github.com/wch/r-source/blob/trunk/src/include/R_ext/RStartup.h#L35

so i'm pretty sure the issue would persist in r-devel, etc.

Contributor

jonathon-love commented Feb 12, 2016

Hm, why are tossing around URLs to a 3rd party site on friggin' GitHub when there is wch's well known mirror?

apparently news hadn't made it to my search engine :P

As you are the one troubled by this, could you possibly try something with all of

looking here, there's still the #ifdef Win32

https://github.com/wch/r-source/blob/trunk/src/include/R_ext/RStartup.h#L35

so i'm pretty sure the issue would persist in r-devel, etc.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Feb 12, 2016

Owner

And FWIW what is currently in the master branch here just passed fine on win-builder.

Owner

eddelbuettel commented Feb 12, 2016

And FWIW what is currently in the master branch here just passed fine on win-builder.

@jonathon-love

This comment has been minimized.

Show comment
Hide comment
@jonathon-love

jonathon-love Feb 12, 2016

Contributor

yup, it's only an issue with -std=c++11

Contributor

jonathon-love commented Feb 12, 2016

yup, it's only an issue with -std=c++11

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Feb 12, 2016

Owner

I am not sure what your point is. Do you think we need

#ifndef WIN32
#ifdef _WIN32
#define WIN32
#endif
#endif

It is not our job to make R itself ready for g++ 4.9.* on Windoze. We look after a particular package here, and it builds with the tools I have at my disposal namely win-builder. You need to submit easier to understand, and more clearly argued, requests for changes.

Owner

eddelbuettel commented Feb 12, 2016

I am not sure what your point is. Do you think we need

#ifndef WIN32
#ifdef _WIN32
#define WIN32
#endif
#endif

It is not our job to make R itself ready for g++ 4.9.* on Windoze. We look after a particular package here, and it builds with the tools I have at my disposal namely win-builder. You need to submit easier to understand, and more clearly argued, requests for changes.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Feb 12, 2016

Contributor

Right, I mean that if @jonathon-love wanted to work around the issues with R's headers locally he could use something like

#ifdef _WIN32
# define WIN32
#endif

#include <RInside.h>

I don't think there's any more changes to be made on the RInside side; R likely will need to change something to accommodate the new toolchain and C++11.

Contributor

kevinushey commented Feb 12, 2016

Right, I mean that if @jonathon-love wanted to work around the issues with R's headers locally he could use something like

#ifdef _WIN32
# define WIN32
#endif

#include <RInside.h>

I don't think there's any more changes to be made on the RInside side; R likely will need to change something to accommodate the new toolchain and C++11.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
Owner

eddelbuettel commented Feb 12, 2016

👍

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Feb 12, 2016

Owner

And C++11 is totally uncharted territory on Windows as we have been stuck at g++ 4.6.* for ages.

There is nothing more to say here, unless I miss something.

Owner

eddelbuettel commented Feb 12, 2016

And C++11 is totally uncharted territory on Windows as we have been stuck at g++ 4.6.* for ages.

There is nothing more to say here, unless I miss something.

@jonathon-love

This comment has been minimized.

Show comment
Hide comment
@jonathon-love

jonathon-love Feb 12, 2016

Contributor

yup, i think this is all rounded off. thanks for that suggestion @kevinushey, i'm going to make use of it.

Contributor

jonathon-love commented Feb 12, 2016

yup, i think this is all rounded off. thanks for that suggestion @kevinushey, i'm going to make use of it.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
Owner

eddelbuettel commented Feb 12, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment