RcppCore / Rcpp

Seamless R and C++ Integration
https://www.rcpp.org
GNU General Public License v2.0
742 stars 212 forks source link

ambigous Vector constructors on x64 #756

Closed bgctw closed 6 years ago

bgctw commented 7 years ago

When installing and compiling package code on Windows x64 platform, R issues the following complaint:

RHLightResponseCostC.cpp:25:46: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second: [enabled by default]
C:/apps/R-3.2.4/library/Rcpp/include/Rcpp/vector/Vector.h:94:5: note: candidate 1: Rcpp::Vector<RTYPE, StoragePolicy>::Vector(const int&, const stored_type&) [with int RTYPE = 10, StoragePolicy = Rcpp::PreserveStorage, Rcpp::Vector<RTYPE, StoragePolicy>::stored_type = int]
C:/apps/R-3.2.4/library/Rcpp/include/Rcpp/vector/Vector.h:152:5: note: candidate 2: Rcpp::Vector<RTYPE, StoragePolicy>::Vector(const Rcpp::Dimension&, const U&) [with U = bool, int RTYPE = 10, StoragePolicy = Rcpp::PreserveStorage]

Although, I tried in my C-code to make it as unambigous as possible:

const bool _fixVPD = fixVPD[0];
const std::size_t _VPDsize = VPD.size();
LogicalVector fixVPD = LogicalVector( _VPDsize, _fixVPD );

R does not complain in the compile run with i386. What is to be changed to prevent the warning, that occurs also as a warnign in R CMD check. I have only little experience with C or Cpp yet.

I performed an update.packages("Rcpp") and got ‘Rcpp’ version 0.12.10 working with R 3.2.4 on Windows 7.

Best regards Thomas

eddelbuettel commented 7 years ago

Could you please try Rcpp 0.12.12 from source by using the appropriate argument to install.packages() ?

0.12.10 is a little dated by our standards -- but not as much ass your R version, and with that I fear your compiler ... but they should all work.

bgctw commented 7 years ago

Dear Dirk, thanks for the quick reply.

After updating Cpp to 12.12 the warning stayes the same.

After updating R to 3.4.1, RTools to 3.4 and updating all the packages, the warning format differs, but is essentially the same:

*** arch - x64
c:/Rtools/mingw_64/bin/g++  -I"C:/apps/R-3.4.1/include" -DNDEBUG  -I"C:/apps/R-3
.4.1/library/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2
-Wall  -mtune=core2 -c RHLightResponseCostC.cpp -o RHLightResponseCostC.o
RHLightResponseCostC.cpp: In function 'Rcpp::NumericVector RHLightResponseCostC(
Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVect
or, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::Numeric
Vector, Rcpp::NumericVector, Rcpp::LogicalVector)':
RHLightResponseCostC.cpp:25:46: warning: ISO C++ says that these are ambiguous,
even though the worst conversion for the first is better than the worst conversi
on for the second:
    fixVPD = LogicalVector( _VPDsize, _fixVPD );
                                              ^
In file included from C:/apps/R-3.4.1/library/Rcpp/include/Rcpp/Vector.h:52:0,
                 from C:/apps/R-3.4.1/library/Rcpp/include/Rcpp.h:40,
                 from RHLightResponseCostC.cpp:1:
C:/apps/R-3.4.1/library/Rcpp/include/Rcpp/vector/Vector.h:94:5: note: candidate
1: Rcpp::Vector<RTYPE, StoragePolicy>::Vector(const int&, const stored_type&) [w
ith int RTYPE = 10; StoragePolicy = Rcpp::PreserveStorage; Rcpp::Vector<RTYPE, S
toragePolicy>::stored_type = int]
     Vector( const int& size, const stored_type& u) {
     ^
C:/apps/R-3.4.1/library/Rcpp/include/Rcpp/vector/Vector.h:152:5: note: candidate
 2: Rcpp::Vector<RTYPE, StoragePolicy>::Vector(const Rcpp::Dimension&, const U&)
 [with U = bool; int RTYPE = 10; StoragePolicy = Rcpp::PreserveStorage]
     Vector( const Dimension& dims, const U& u) {
     ^

Thanks, Thomas

eddelbuettel commented 7 years ago

What happens if you use an IntegerVector instead of a LogicalVector?

(And overall, it's still just a warning...)

eddelbuettel commented 7 years ago

For what it is worth I do not these on Ubuntu 17.04 with g++-6.3:

/tmp/git/REddyProc(master)$ R_LIBS_USER=/tmp/RcppDepends/lib R CMD INSTALL .
* installing to library ‘/tmp/RcppDepends/lib’
* installing *source* package ‘REddyProc’ ...
** libs
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -Wno-unused -pedantic -Werror -march=native -c RHLightResponseCostC.cpp -o RHLightResponseCostC.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -Wno-unused -pedantic -Werror -march=native -c RcppExports.cpp -o RcppExports.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -Wno-unused -pedantic -Werror -march=native -c whichValueGreaterEqualC.cpp -o whichValueGreaterEqualC.o
ccache g++ -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -o REddyProc.so RHLightResponseCostC.o RcppExports.o whichValueGreaterEqualC.o -L/usr/lib/R/lib -lR
installing to /tmp/RcppDepends/lib/REddyProc/libs
** R
** data
** inst
** preparing package for lazy loading
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded
* DONE (REddyProc)
/tmp/git/REddyProc(master)$

I do use -pedantic here which tends to warn a lot. I use Rcpp 0.12.12.5 (a prerelease) but that should not matter either.

bgctw commented 6 years ago

I found the solution: std:size_t caused the confusion. When I replace it by int, it works fine:

const bool _fixVPD = fixVPD[0];
// causes ambiguities: const std::size_t _VPDsize = VPD.size();
const int _VPDsize = VPD.size();
fixVPD = LogicalVector( _VPDsize, _fixVPD );

I am not enough C-expert to assess the consequences and hope it will not break on future platforms. To my understanding the only small drawbacks are a potentially unnecessary conversion or that int might not be big enough.

Just to note: although there were only warnings, they prevented the package from install on winbuilder and from submission to CRAN:

*** arch - x64
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.5/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -mtune=core2 -c RHLightResponseCostC.cpp -o RHLightResponseCostC.o
RHLightResponseCostC.cpp: In function 'Rcpp::NumericVector RHLightResponseCostC(Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::NumericVector, Rcpp::LogicalVector)':
RHLightResponseCostC.cpp:25:46: error: call of overloaded 'Vector(const size_t&, const bool&)' is ambiguous
    fixVPD = LogicalVector( _VPDsize, _fixVPD );
                                              ^
RHLightResponseCostC.cpp:25:46: note: candidates are:
In file included from d:/RCompile/CRANpkg/lib/3.5/Rcpp/include/Rcpp/Vector.h:52:0,
                 from d:/RCompile/CRANpkg/lib/3.5/Rcpp/include/Rcpp.h:40,
                 from RHLightResponseCostC.cpp:1:
d:/RCompile/CRANpkg/lib/3.5/Rcpp/include/Rcpp/vector/Vector.h:152:5: note: Rcpp::Vector<RTYPE, StoragePolicy>::Vector(const Rcpp::Dimension&, const U&) [with U = bool; int RTYPE = 10; StoragePolicy = Rcpp::PreserveStorage]
     Vector( const Dimension& dims, const U& u) {
     ^
d:/RCompile/CRANpkg/lib/3.5/Rcpp/include/Rcpp/vector/Vector.h:94:5: note: Rcpp::Vector<RTYPE, StoragePolicy>::Vector(const int&, const stored_type&) [with int RTYPE = 10; StoragePolicy = Rcpp::PreserveStorage; Rcpp::Vector<RTYPE, StoragePolicy>::stored_type = int]
     Vector( const int& size, const stored_type& u) {
     ^
make: *** [RHLightResponseCostC.o] Error 1
ERROR: compilation failed for package 'REddyProc'
* removing 'd:/RCompile/CRANguest/R-devel/lib/REddyProc'
eddelbuettel commented 6 years ago

So are we concluding that we change was in your package, and not in Rcpp? If that is the case, shall we close this?

bgctw commented 6 years ago

The change in the package is more a workaround the real problem, as std::size_t is the correct type, but I close it.

eddelbuettel commented 6 years ago

As before, I am somewhat confused why a) this does not come up with other packages and b) what it would have to do with Windows in your case.

Very strange. But closing is the right thing. If new information comes to light we can reopen.

bgctw commented 6 years ago

Unfortunately, my workaround of casting to int fails dangerously. When I install the package on our compute cluster (mostly running software versions several month behind) I get a segmentation fault:

twutz@io1 REddyProcWeb> R32 CMD INSTALL -l ~/REddyProcWeb/Library ~/twutz/eclipse_R/REddyProc
* installing *source* package 'REddyProc' ...
** libs
g++ -I/usr/local/apps/R/R-3.2.2/lib64/R/include -DNDEBUG  -I/usr/local/include -I"/User/homes/twutz/R/x86_64-pc-linux-gnu-library/3.2/Rcpp/include"   -fpic  -g -O2  -c RHLightResponseCostC.cpp -o RHLightResponseCostC.o
g++ -I/usr/local/apps/R/R-3.2.2/lib64/R/include -DNDEBUG  -I/usr/local/include -I"/User/homes/twutz/R/x86_64-pc-linux-gnu-library/3.2/Rcpp/include"   -fpic  -g -O2  -c RcppExports.cpp -o RcppExports.o
g++ -shared -L/usr/local/apps/R/R-3.2.2/lib64/R/lib -L/usr/local/lib64 -o REddyProc.so RHLightResponseCostC.o RcppExports.o whichValueGreaterEqualC.o -L/usr/local/apps/R/R-3.2.2/lib64/R/lib -lR
installing to /Net/Groups/BGI/services/REddyProcWeb/Library/REddyProc/libs
** R
** data
*** moving datasets to lazyload DB
** inst
** preparing package for lazy loading
Warning: replacing previous import by 'tibble::as_tibble' when loading 'REddyProc'
Warning: replacing previous import by 'tibble::tibble' when loading 'REddyProc'
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded
sh: line 1: 11083 Segmentation fault      '/usr/local/apps/R/R-3.2.2/lib64/R/bin/R' --no-save --slave 2>&1 < '/scratch/tmp/RtmpY3OsLn/file2ac63956e008'
Warning: replacing previous import by 'tibble::as_tibble' when loading 'REddyProc'
Warning: replacing previous import by 'tibble::tibble' when loading 'REddyProc'

 *** caught segfault ***
address (nil), cause 'memory not mapped'

Traceback:
 1: dyn.load(file, DLLpath = DLLpath, ...)
 2: library.dynam(lib, package, package.lib)
 3: loadNamespace(package, c(which.lib.loc, lib.loc))
 4: doTryCatch(return(expr), name, parentenv, handler)
 5: tryCatchOne(expr, names, parentenv, handlers[[1L]])
 6: tryCatchList(expr, classes, parentenv, handlers)
 7: tryCatch(expr, error = function(e) {    call <- conditionCall(e)    if (!is.null(call)) {        if (identical(call[[1L]], quote(doTryCatch)))             call <- sys.call(-4L)        dcall <- deparse(call)[1L]        prefix <- paste("Error in", dcall, ": ")        LONG <- 75L        msg <- conditionMessage(e)        sm <- strsplit(msg, "\n")[[1L]]        w <- 14L + nchar(dcall, type = "w") + nchar(sm[1L], type = "w")        if (is.na(w))             w <- 14L + nchar(dcall, type = "b") + nchar(sm[1L],                 type = "b")        if (w > LONG)             prefix <- paste0(prefix, "\n  ")    }    else prefix <- "Error : "    msg <- paste0(prefix, conditionMessage(e), "\n")    .Internal(seterrmessage(msg[1L]))    if (!silent && identical(getOption("show.error.messages"),         TRUE)) {        cat(msg, file = stderr())        .Internal(printDeferredWarnings())    }    invisible(structure(msg, class = "try-error", condition = e))})
 8: try({    ns <- loadNamespace(package, c(which.lib.loc, lib.loc))    env <- attachNamespace(ns, pos = pos, deps)})
 9: library(pkg_name, lib.loc = lib, character.only = TRUE, logical.return = TRUE)
10: withCallingHandlers(expr, packageStartupMessage = function(c) invokeRestart("muffleMessage"))
11: suppressPackageStartupMessages(library(pkg_name, lib.loc = lib,     character.only = TRUE, logical.return = TRUE))
12: doTryCatch(return(expr), name, parentenv, handler)
13: tryCatchOne(expr, names, parentenv, handlers[[1L]])
14: tryCatchList(expr, classes, parentenv, handlers)
15: tryCatch(expr, error = function(e) {    call <- conditionCall(e)    if (!is.null(call)) {        if (identical(call[[1L]], quote(doTryCatch)))             call <- sys.call(-4L)        dcall <- deparse(call)[1L]        prefix <- paste("Error in", dcall, ": ")        LONG <- 75L        msg <- conditionMessage(e)        sm <- strsplit(msg, "\n")[[1L]]        w <- 14L + nchar(dcall, type = "w") + nchar(sm[1L], type = "w")        if (is.na(w))             w <- 14L + nchar(dcall, type = "b") + nchar(sm[1L],                 type = "b")        if (w > LONG)             prefix <- paste0(prefix, "\n  ")    }    else prefix <- "Error : "    msg <- paste0(prefix, conditionMessage(e), "\n")    .Internal(seterrmessage(msg[1L]))    if (!silent && identical(getOption("show.error.messages"),         TRUE)) {        cat(msg, file = stderr())        .Internal(printDeferredWarnings())    }    invisible(structure(msg, class = "try-error", condition = e))})
16: try(suppressPackageStartupMessages(library(pkg_name, lib.loc = lib,     character.only = TRUE, logical.return = TRUE)))
17: tools:::.test_load_package("REddyProc", "/Net/Groups/BGI/services/REddyProcWeb/Library")
aborting ...
ERROR: loading failed
* removing '/Net/Groups/BGI/services/REddyProcWeb/Library/REddyProc'
* restoring previous '/Net/Groups/BGI/services/REddyProcWeb/Library/REddyProc'

When I switch back to std::size_t, it works on the cluster node. The cluster node is running a GNU/linux:

Linux io1 3.0.101-0.47.71-default #1 SMP Thu Nov 12 12:22:22 UTC 2015 (b5b212e) x86_64 x86_64 x86_64 GNU/Linux

eddelbuettel commented 6 years ago

Come on. That is your packages issiue, not Rcpp. Make the code an `#ifdef``. There are example tests for Windows use all over the source.

I still suspect it is masking a different issue but there is nothing reproducible here.