-
-
Notifications
You must be signed in to change notification settings - Fork 209
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 intel icc compilation of algorithm.h (closes 640) #643
Conversation
Codecov Report@@ Coverage Diff @@
## master #643 +/- ##
======================================
Coverage 92.5% 92.5%
======================================
Files 65 65
Lines 3309 3309
======================================
Hits 3061 3061
Misses 248 248 Continue to review full report at Codecov.
|
Looks good to me, given the discussion in #640. |
Still providing similar warnings. Just ran this branch on ICC with Intel Compiler 15.0.3/16.0/17.0 on R 3.2.2. Output across versions was: > devtools::install_github("rcppcore/rcpp", ref="bugfix/issue640", force=TRUE)
Downloading GitHub repo rcppcore/rcpp@bugfix/issue640
from URL https://api.github.com/repos/rcppcore/rcpp/zipball/bugfix/issue640
Installing Rcpp
'/usr/local/R/R-3.2.2/lib64/R/bin/R' --no-site-file --no-environ --no-save \
--no-restore CMD INSTALL \
'/tmp/RtmpWjmcXR/devtools9e5b4c9d99a5/RcppCore-Rcpp-98ecceb' \
--library='/home/balamut2/Rlibs' --install-tests
* installing *source* package 'Rcpp' ...
** libs
icc -I/usr/local/R/R-3.2.2/lib64/R/include -DNDEBUG -I../inst/include/ -I/usr/local/include -fpic -g -O2 -c Date.cpp -o Date.o
In file included from ../inst/include/Rcpp.h(86),
from Date.cpp(31):
../inst/include/Rcpp/algorithm.h(177): warning #858: type qualifier on return type is meaningless
static inline RCPP_CONSTEXPR double ZERO() { return 0.0; }
^ |
Could you run the Intel tests again, but this time enable C++11 with the argument -std=C++11 ? I think you do not have 11 enabled in your tests. |
Aye, when I went to build against the Rcpp version I opted for the default settings (e.g. no Switching over to compiling with C++11 did lead to just one warning that was present before:
|
Looks much better now. Thanks for rerunning. |
That is sadly not good enough. Rcpp itself is not built using While we allow it it client packages (and eg haven may be using), we do not use it ourselves to only require a more basic compiler. g++-4.6.* and g++-4.4.* are still used on ancient RH systems. Now we did not get the warning there yet. Do we need a third branch in that |
If you want to shut up Intel in non c++11 mode, then I am afraid that you need another fix for the else branch. Optimally one can figure out a way which makes Intel and gcc happy; otherwise we need as you say another Intel quirk in there which I can try to find out what ifdef is needed to detect if Intel is used. |
Or we just merge as is, and the pleasure of using Not the end of the world. |
Fine with me. Maybe I can look at it some time, now that I have a direct way to trigger the warning. After all its a warning, no error; so feel free to proceed. |
I made the change adding a simple test for the intel compiler. Will commit, and a quick review should let this in so that we close this chapter. |
Re-upping poke to @kevinushey @nathan-russell @dcdillon @jjallaire @thirdwing @coatless: All good? |
So, forcing it to be blank under the Intel compiler actually triggers an error and kills the compilation. Seems like to quiet this the value should not be defined in the declaration but in the definition per C++ a member with an in-class initializer must be const.
|
Thanks so much for testing, @coatless. The current state is clearly worse. I think your SO makes it clear that @kevinushey was (as usual) right: we need to differentiate between use of I think for now I am going to revert my last commit, and the merge as it was. It will build everywhere, but be noisy under icc when C++11 is not on (which folks could always turn locally I presume). |
This reverts commit f17ee6a.
Simple PR adding what #640 ended up with in conclusion
Comments welcome, in particular double-checking whether the
else
side of the#if
should be empty orconst
. Per #640 empty did not work.Poking @kevinushey @nathan-russell @dcdillon @jjallaire @thirdwing