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

allow long long on 64bit windows #811

Merged
merged 1 commit into from Feb 8, 2018

Conversation

Projects
None yet
3 participants
@kevinushey
Contributor

kevinushey commented Feb 7, 2018

Fixes #804. Because the compiler toolchain on Windows provides long long regardless of whether C++98 or a newer standard is requested, this usage should be permissible on CRAN.

In addition, because R_xlen_t and std::size_t are actually defines for long long and unsigned long long on 64bit Windows, those constructors now work 'for free'.

@eddelbuettel eddelbuettel merged commit dc7900b into master Feb 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@krlmlr

This comment has been minimized.

Contributor

krlmlr commented Feb 8, 2018

Thanks @kevinushey for looking into it!

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 7, 2018

@kevinushey on win-builder r-devel I get:

* checking whether package 'Rcpp' can be installed ... WARNING
Found the following significant warnings:
  ../inst/include/Rcpp/longlong.h:43:14: warning: ISO C++ 1998 does not support 'long long' [-Wlong-long]
  ../inst/include/Rcpp/longlong.h:44:23: warning: ISO C++ 1998 does not support 'long long' [-Wlong-long]
See 'd:/RCompile/CRANguest/R-devel/Rcpp.Rcheck/00install.out' for details.
* checking installed package size ... 

Do we need additional C++11 protection around these?

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Mar 7, 2018

grumbles loudly

Let me think about whether there's an alternate mechanism by which we can get a 64bit integer type on Windows. I'd love for this to work even without C++11 but worst case scenario we can just add that in.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 7, 2018

I am 100% certain that we fought these dragons many times before. And at some point I just gave it a major fsck this as you cannot argue away the sillyness of the is C++ 1998 (hey, twenty years) requirement for an entirely irrelevant standard.

But long and short, unless users opt into C++11 (as eg my RcppBDT for Boost date_time did maybe six years ago) to get long long they simply will not get them on 'doze. C'est la vie

I would be delighted to be proven wrong but don't expect it.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 7, 2018

Ok, win-builder came back with a thumbs up and I suggest that we go fix this fix:

modified   inst/include/Rcpp/longlong.h
@@ -40,9 +40,11 @@ __extension__ typedef unsigned long long int rcpp_ulong_long_type;
 
 // The Rtools toolchain provides long long on 64bit Windows
 #ifdef _WIN64
+#ifdef RCPP_USING_CXX0X_OR_LATER
 typedef long long rcpp_long_long_type;
 typedef unsigned long long rcpp_ulong_long_type;
 #define RCPP_HAS_LONG_LONG_TYPES
 #endif
+#endif

Any objections? I will be less 'global' that what @krlmlr had hoped but such is life when our good friends insist on C++98 for reasons way beyond our comprehension.

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Mar 7, 2018

I'm going to take another look at this so may follow up with another PR.

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Mar 7, 2018

(but if I fail I think that will be the right change)

@krlmlr

This comment has been minimized.

Contributor

krlmlr commented Mar 7, 2018

Can we just set CXX_STD=CXX11 in Makevars.win? Worked for me in RSQLite and friends, I'm using int64_t there without problems. (Thanks @jeroen for the tip!)

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 7, 2018

@krlmlr Yes the 'use C++11' trick has worked (and been used) for many many years.

@kevinushey ok

@krlmlr

This comment has been minimized.

Contributor

krlmlr commented Mar 7, 2018

Do I understand correctly that we can't just add in CXX_STD=CXX11 to Makevars.win in Rcpp? Or is it about packages having to opt in to use C++11?

Can we add some protection to avoid the underlying issue to occur in the future? I'd like to see an error or a warning if a vector constructor is called with an R_xlen_t and this is silently coerced to a 32-bit integer.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 7, 2018

We didn't do this for Rcpp as we would then force down a newer compiler. @jjallaire usually reminds us of the numerous old RHEL instances still trying to build it.

We impose a new(er) compiler for RcppArmadillo and get occassional issues back, see its issue tickets.

As for R_xlen_t and 32-bit: that is a) an R issue I believe them to handle well enough and b) on the other hand 32 bit machines are on their way out too... The main issue, really, is Windows where we now have g++ 4,9.3 which while far from ideal is a huge step up from the 4.6.2 dinosaur before it.

@krlmlr

This comment has been minimized.

Contributor

krlmlr commented Mar 7, 2018

I'm only suggesting to enable C++11 on Windows. Would that work?

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Mar 7, 2018

Maybe. But I don't like the idea of splitting standards. And 4.9.3 is not really C++11 compliant, it just pretends to be. (For RcppCCTZ we ported something from the C++ Std Lib back.)

And the patch by @kevinushey is functionally equivalent.

@eddelbuettel eddelbuettel deleted the bugfix/windows-64bit-long-long branch Mar 8, 2018

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