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

Add variadic variants of the RCPP_RETURN_VECTOR/MATRIX macro #537

Merged
merged 5 commits into from
Aug 11, 2016
Merged

Add variadic variants of the RCPP_RETURN_VECTOR/MATRIX macro #537

merged 5 commits into from
Aug 11, 2016

Conversation

artemklevtsov
Copy link
Contributor

@artemklevtsov artemklevtsov commented Aug 9, 2016

Add conditional definition of the RCPP_RETURN_VECTOR and RCPP_RETURN_MATRIX macro (depends of the RCPP_USING_CXX11). Also added unit tests for this macro.
This PR fixes #38.
Possible use-case: https://gist.github.com/artemklevtsov/5e1155551d90196adacb86ae80546588

P.S.: How can I test the variadic variants of the macro?

@eddelbuettel
Copy link
Member

eddelbuettel commented Aug 9, 2016

Thanks for the PR.

Don't pile on extra commit while the tests still run -- this failed (now twice), so so far you are breaking master which is of course a reason for us to not merge. Please fix that you in your PR.

What is your test environment, and why do you think R CMD check passed for you? You ran that before committing, right?

@artemklevtsov
Copy link
Contributor Author

artemklevtsov commented Aug 9, 2016

My bad, sorry. Seems some tests cause an errors in others. Now all tests passed successful.
Still I don't know how to write unit tests for the C++11 features. May be something like this:

#include <Rcpp.h>

using namespace Rcpp;

// [[Rcpp::plugins("cpp11")]]

template <typename T>
T get_elem_impl(const T& x, std::size_t i) {
    T res(1);
    res[0] = x.at(i);
    return res;
}

// [[Rcpp::export]]
RObject get_elem(RObject vec, std::size_t i) {
    RCPP_RETURN_VECTOR(get_elem_impl, vec, i);
}

/***R
get_elem(1:5, 3)
get_elem(letters, 15)
*/

But how can I use it with RUnit?

@eddelbuettel
Copy link
Member

Good -- though I glanced at your commit earlier and it seems like you simply disabled two tests.

That may warrant a slightly wider discussion. Can we not get Expression(Vector|Matrix) back and have new variadic variants?

@artemklevtsov
Copy link
Contributor Author

artemklevtsov commented Aug 9, 2016

Expression(Vector/Matrix) passed correctly (checkEquals is TRUE) but it breaks other tests. I do not know why. May be I wrote the incorrect data for example.
Note, before these macros are not tested at all.

@eddelbuettel
Copy link
Member

I do not follow. You commented out Expression(Vector/Matrix) in that last commit. If you view the display linked to above it is red -- removed code. Now it passes.

The basic idea of unit tests is that new code (like your commit) does not require changes to existing tests (and behaviour). That is a very useful idea. I'd like to stick wit it.

@artemklevtsov
Copy link
Contributor Author

I agree. Now I don't understand why it happens. A possible reason may be match a function names in .runThisTest with other tests.

@eddelbuettel
Copy link
Member

eddelbuettel commented Aug 9, 2016

We're on the same page.

Time permitting I can try to clone your repo and take a look. No guarantee I get to it soon though :-/

@eddelbuettel
Copy link
Member

Looks much cleaner now.

Testing with C++11 is indeed a bit tricky. Not sure I gave a solution to offer. Anybody? The plugin trick may just work as the files are sourceCpp()-ed.

@eddelbuettel
Copy link
Member

Come to think about it, you probably want two unit test files for C++, one with the C++11 plugin and one without. But we can do that next round -- will merge this now.

@eddelbuettel eddelbuettel merged commit 01c8f2f into RcppCore:master Aug 11, 2016
@artemklevtsov
Copy link
Contributor Author

Thank you for the time and suggestions.

@eddelbuettel
Copy link
Member

If you have a test file with a // [[Rcpp::Depends("cpp11")]], say inst/unitTests/cpp/dispatch_cpp11.cpp, we could add that to test variadic args. Can you cook something up?

@artemklevtsov
Copy link
Contributor Author

artemklevtsov commented Aug 12, 2016

I thought about it. Let me show a simple draft:

#ifdef RCPP_USING_CXX11
// [[Rcpp::plugins("cpp11")]]
template <int RTYPE>
Vector<RTYPE> get_el_impl(const Vector<RTYPE>& x, std::size_t i) {
    Vector<RTYPE> res(1);
    res[0] = x.at(i);
    return res;
}

// [[Rcpp::export]]
SEXP get_el(SEXP x, std::size_t i) {
    RCPP_RETURN_VECTOR(get_el_impl, x, i);
}

template <int RTYPE>
Vector<RTYPE> get_cell_impl(const Matrix<RTYPE>& x, std::size_t i, std::size_t j) {
    Vector<RTYPE> res(1);
    res[0] = x(i, j);
    return res;
}

// [[Rcpp::export]]
SEXP get_cell(SEXP x, std::size_t i, std::size_t j) {
    RCPP_RETURN_MATRIX(get_cell_impl, x, i, j)
}

#endif

But what about runit.dispatch_cpp11.R? Should I use the .runThisTest && Rcpp:::capabilities()[["Full C++11 support"]] condition?

@eddelbuettel
Copy link
Member

I am thinking about it slightly differently:

  • tests are running from R unconditionally
  • all R files are being hit the same way
  • so by placing the C++11 plugin into a file we get that file to compile with c++11

So that covers the 'test C++11 when we have it' case.

But you make you good case that we need to cover the fallback case. I am not yet quite sure what to do there. Maybe we need something like capabilities() but just accessing something at test time. Maybe a helper returning TRUE or FALSE depending on whether C++11 builds succeeded. Not sure yet.

@eddelbuettel
Copy link
Member

eddelbuettel commented Aug 12, 2016

BTW the Rcpp:::capabilties() test is not helpful; that may always be false (as it is on my box where C++11 can still be turned on via plugins).

@artemklevtsov
Copy link
Contributor Author

artemklevtsov commented Aug 13, 2016

I think we can use the exists("get_cell", globalenv()) condition in the runit.dispatch_cpp11.R right now.

@jayhesselberth
Copy link

This new RCPP_RETURN_VECTOR is giving me lots of warnings (using -pedantic -Wall) during compilation of a package (which depends on dplyr). I am still trying to figure things out on my end, but my question is whether GNU extensions are usually included in Rcpp? I am thinking in terms of portability.

The errors can be recreated by adding SystemRequirements: C++11 to the DESCRIPTION file in a fork of dplyr.

clang++ -std=c++11 -I/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/include -DNDEBUG -I../inst/include -DCOMPILING_DPLYR -DBOOST_NO_INT64_T -DBOOST_NO_INTEGRAL_INT64_T -DBOOST_NO_LONG_LONG -I/usr/local/opt/gettext/include -I/usr/local/opt/readline/include -I/usr/local/opt/openssl/include -I/usr/local/include -I"/usr/local/lib/R/3.3/site-library/Rcpp/include" -I"/usr/local/lib/R/3.3/site-library/BH/include" -I"/usr/local/lib/R/3.3/site-library/plogr/include"  -Wall -pedantic -fPIC  -g -O2 -c summarise.cpp -o summarise.o
In file included from summarise.cpp:1:
In file included from ../inst/include/dplyr/main.h:4:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp.h:27:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/RcppCommon.h:29:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/r/headers.h:50:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/macros.h:82:
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:56:42: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ___RCPP_RETURN___(_FUN_, _SEXP_, Vector, ##__VA_ARGS__)
                                         ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:35:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(INTSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^

@eddelbuettel
Copy link
Member

BTW @artemklevtsov I just noticed that you copied the unittest files without updating the file header; they still said "(C) Romain, 2013". Don't do that. If you copy from an existing file, it is polite to keep the old value -- put your name in it and the current year as well. It mislead me the other day when I looked at the file.

@eddelbuettel
Copy link
Member

eddelbuettel commented Oct 27, 2016

I use -pedantic -Wall as default as well, but I do not use dplyr. My builds are all clean; we tend to get rid of warnings etc as they arise.

Maybe you need to talk to them rather than us? Could be that their code now needs an update.

@jayhesselberth
Copy link

I note that this may cause issues down the line. Maybe something changed recently, but as of Dec 2015, using __VA_ARGS__ was not recommended because of CRAN's requirement for C++98 compliance:

Quoting from: #407

One of the really unfortunate issues with VA_ARGS (or, variadic macros in general) is that,
strictly speaking, it is not part of the C++98 standard, which Rcpp must conform to (given the
CRAN requirements). Even though, in practice, every compiler implements it as it is part of the >C99 standard ...

then

Doh. I forgot about that--the reason we do not use VA_ARGS in the other scripted / macro-created interfaces.

@eddelbuettel
Copy link
Member

Earlier you said (which I missed)

whether GNU extensions are usually included in Rcpp

and the answer is: no, not unconditionally. Because doing that would not be portable, and some of the folks over at CRAN care vary deeply about (and, I have to say, mostly for a good reason).

So look again. I am sure the use of something violating C++98 is #ifdef'ed by tests for C++11 which we can now (at last!) turn on.

I have to rush out to do some Software Carpentry teaching but if you think you found that we violate that idea somewhere then I'd love to know more. URLs with links to code preferred. Thanks!

@jayhesselberth
Copy link

Dug into this a bit more. Everything is #ifdef'ed correctly (I think). The problem is the use of the GNU-specific extension.

My understanding is that the following (pasted from above) is expected to compile (using sourceCpp, under 0.12.7) without warnings. It does not. (my ~.R/Makevars has PKG_CPPFLAGS = -pedantic -Wall). Can you confirm?

#include <Rcpp.h>

using namespace Rcpp;

// [[Rcpp::plugins("cpp11")]]

template <typename T>
  T get_elem_impl(const T& x, std::size_t i) {
    T res(1);
    res[0] = x.at(i);
    return res;
  }

// [[Rcpp::export]]
RObject get_elem(RObject vec, std::size_t i) {
  RCPP_RETURN_VECTOR(get_elem_impl, vec, i);
}

/***R
get_elem(1:5, 3)
get_elem(letters, 15)
*/

These warnings are not generated during an Rcpp build. And none of tests in this PR test the variadic macro (they only have one argument), nor are they built with the cpp11 plugin. If you turn on cpp11 (e.g., in the example above), you get warnings.

Do you have a suggestion for a way forward? Or a fix?

@eddelbuettel
Copy link
Member

eddelbuettel commented Nov 3, 2016

Clean as a whistle for me. Ubuntu 16.04, g++ 5.4.0, similar default flags, Rcpp from master:

R> sourceCpp("/tmp/jay.cpp", verbose=TRUE)

Generated extern "C" functions 
--------------------------------------------------------


#include <Rcpp.h>
// get_elem
RObject get_elem(RObject vec, std::size_t i);
RcppExport SEXP sourceCpp_8_get_elem(SEXP vecSEXP, SEXP iSEXP) {
BEGIN_RCPP
    Rcpp::RObject rcpp_result_gen;
    Rcpp::RNGScope rcpp_rngScope_gen;
    Rcpp::traits::input_parameter< RObject >::type vec(vecSEXP);
    Rcpp::traits::input_parameter< std::size_t >::type i(iSEXP);
    rcpp_result_gen = Rcpp::wrap(get_elem(vec, i));
    return rcpp_result_gen;
END_RCPP
}

Generated R functions 
-------------------------------------------------------

`.sourceCpp_8_DLLInfo` <- dyn.load('/tmp/RtmpOW9d9Z/sourceCpp-x86_64-pc-linux-gnu-0.12.7.5/sourcecpp_2eab63f3ae05/sourceCpp_9.so')

get_elem <- Rcpp:::sourceCppFunction(function(vec, i) {}, FALSE, `.sourceCpp_8_DLLInfo`, 'sourceCpp_8_get_elem')

rm(`.sourceCpp_8_DLLInfo`)

Building shared library
--------------------------------------------------------

DIR: /tmp/RtmpOW9d9Z/sourceCpp-x86_64-pc-linux-gnu-0.12.7.5/sourcecpp_2eab63f3ae05

/usr/lib/R/bin/R CMD SHLIB -o 'sourceCpp_9.so'  'jay.cpp'  
ccache g++ -std=c++11 -I/usr/share/R/include -DNDEBUG    -I"/usr/local/lib/R/site-library/Rcpp/include" -I"/tmp"    -fpic  -g -O3 -Wall -pipe -Wno-unused -pedantic -c jay.cpp -o jay.o
ccache g++ -std=c++11 -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -o sourceCpp_9.so jay.o -L/usr/lib/R/lib -lR

R> get_elem(1:5, 3)
[1] 4

R> get_elem(letters, 15)
[1] "p"
R> 

@eddelbuettel
Copy link
Member

eddelbuettel commented Nov 3, 2016

As for "but I think you guys are bad boys using GNU extensions" I can only assure you that a certain (rather famous) (and now retired) Oxford professor is still extremely vigilant about these things.

You can in general assume CRAN packages to be behaving correctly.

@nathan-russell
Copy link
Contributor

The issue is not with the use of __VA_ARGS__ per se, but specifically the token pasting of commas with __VA_ARGS__ in places like this:

___RCPP_RETURN___(_FUN_, _SEXP_, Vector, ##__VA_ARGS__)
                                       ^^^^^    

There is possibly a solution involving a fair bit of macro trickery -- see this SO post -- but you would have to spend some time exploring that route to know for sure as it is a little beyond my comfort zone WRT working with macros.

@jayhesselberth
Copy link

I'll leave this here for the next person (maybe me, when I revisit) that runs into it:

> sourceCpp('/tmp/dirk.cpp', verbose = TRUE)

Generated extern "C" functions 
--------------------------------------------------------


#include <Rcpp.h>
// get_elem
RObject get_elem(RObject vec, std::size_t i);
RcppExport SEXP sourceCpp_1_get_elem(SEXP vecSEXP, SEXP iSEXP) {
BEGIN_RCPP
    Rcpp::RObject rcpp_result_gen;
    Rcpp::RNGScope rcpp_rngScope_gen;
    Rcpp::traits::input_parameter< RObject >::type vec(vecSEXP);
    Rcpp::traits::input_parameter< std::size_t >::type i(iSEXP);
    rcpp_result_gen = Rcpp::wrap(get_elem(vec, i));
    return rcpp_result_gen;
END_RCPP
}

Generated R functions 
-------------------------------------------------------

`.sourceCpp_1_DLLInfo` <- dyn.load('/private/var/folders/_1/4wg2xbj12_dft9p3kq4005n40000gn/T/RtmpBPL40n/sourceCpp-x86_64-apple-darwin15.6.0-0.12.7/sourcecpp_16257318ad455/sourceCpp_2.so')

get_elem <- Rcpp:::sourceCppFunction(function(vec, i) {}, FALSE, `.sourceCpp_1_DLLInfo`, 'sourceCpp_1_get_elem')

rm(`.sourceCpp_1_DLLInfo`)

Building shared library
--------------------------------------------------------

DIR: /private/var/folders/_1/4wg2xbj12_dft9p3kq4005n40000gn/T/RtmpBPL40n/sourceCpp-x86_64-apple-darwin15.6.0-0.12.7/sourcecpp_16257318ad455

/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/bin/R CMD SHLIB -o 'sourceCpp_2.so'  'dirk.cpp'  
clang++ -std=c++11 -I/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/include -DNDEBUG  -I/usr/local/opt/gettext/include -I/usr/local/opt/readline/include -I/usr/local/opt/openssl/include -I/usr/local/include  -I"/usr/local/lib/R/3.3/site-library/Rcpp/include" -I"/private/tmp"   -pedantic -Wall -fPIC  -g -O2 -c dirk.cpp -o dirk.o
In file included from dirk.cpp:1:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp.h:27:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/RcppCommon.h:29:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/r/headers.h:50:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/macros.h:82:
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:56:44: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ___RCPP_RETURN___(_FUN_, _SEXP_, Vector, ##__VA_ARGS__)
                                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:36:28: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                           ##__VA_ARGS__)                                      \
                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:38:28: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                           ##__VA_ARGS__)                                      \
                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:40:28: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                           ##__VA_ARGS__)                                      \
                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:42:28: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                           ##__VA_ARGS__)                                      \
                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:44:28: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                           ##__VA_ARGS__)                                      \
                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:46:28: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                           ##__VA_ARGS__)                                      \
                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:48:28: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                           ##__VA_ARGS__)                                      \
                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:50:28: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                           ##__VA_ARGS__)                                      \
                           ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:22: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
                     ##__VA_ARGS__);
                     ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:22: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:22: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:22: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:22: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:22: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:22: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:22: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
17 warnings generated.
clang++ -std=c++11 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/usr/local/opt/gettext/lib -L/usr/local/opt/readline/lib -L/usr/local/opt/openssl/lib -L/usr/local/lib -L/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/lib -L/usr/local/opt/gettext/lib -L/usr/local/opt/readline/lib -L/usr/local/opt/openssl/lib -L/usr/local/lib -o sourceCpp_2.so dirk.o -F/usr/local/Cellar/r/3.3.1_3/R.framework/.. -framework R -lintl -Wl,-framework -Wl,CoreFoundation

> get_elem(1:5, 3)
[1] 4

> get_elem(letters, 15)
[1] "p"

@eddelbuettel
Copy link
Member

eddelbuettel commented Nov 3, 2016

Can you please consider upgrading to

R> packageVersion("Rcpp")
[1] ‘0.12.7.5R> 

which is available at the very same venue you use to file these reports: GitHub. And/or replacing clang++ with g++. Half-joking of course: but what is this? 0.12.7 vs 0.12.7.5, or clang++ vs g++ ? I can't tell. I don't get the warning.

@eddelbuettel
Copy link
Member

Ok, it is also silent for with Rcpp 0.12.7 so I'll remove myself from the discussion for lack of a working clang++ (outside of Docker).

@jayhesselberth
Copy link

Latest on github is 0.12.7.3. Is 0.12.7.5 a local (not yet pushed) version?

Same warnings using both clang++ and g++ (via homebrew) on OS X with 0.12.7.3 installed.

@eddelbuettel
Copy link
Member

Yes, sorry, my bad. 0.12.7.5 is a currently "only local" label I used last week when we had a battery of rev.dep runs. You are correct about 0.12.7.3 being master.

As for shutting the macro up: not sure. @nathan-russell had a good suggestion. But it will take someone with a bit of patience and spare time to work that in. Would you volunteer?

@eddelbuettel
Copy link
Member

Also: @artemklevtsov , any comment from your side?

@nathan-russell
Copy link
Contributor

I'm a little surprised that GCC gives this warning on OS X. On RedHat GCC does not warn, but clang does.

@eddelbuettel
Copy link
Member

Ok, two minor cleanups in 8715b35: rolling of the minor version (thanks, Jay) and adjusting the copyright header of dispatch.h by adding Artem.

@eddelbuettel
Copy link
Member

@jayhesselberth Could we possibly lean on you for one or two more test runs? Would the same warnings be triggered by 0.12.6 (which should be the last release before we added this PR) ?

@jayhesselberth
Copy link

Here is the result of compiling dispatch.cpp under 0.12.6 (no warnings).

> library(Rcpp)
> packageVersion('Rcpp')
[1] '0.12.6'
> Rcpp::sourceCpp('/tmp/dispatch.cpp', verbose = TRUE)

Generated extern "C" functions 
--------------------------------------------------------


#include <Rcpp.h>
// first_el
SEXP first_el(SEXP x);
RcppExport SEXP sourceCpp_1_first_el(SEXP xSEXP) {
BEGIN_RCPP
    Rcpp::RObject __result;
    Rcpp::RNGScope __rngScope;
    Rcpp::traits::input_parameter< SEXP >::type x(xSEXP);
    __result = Rcpp::wrap(first_el(x));
    return __result;
END_RCPP
}
// first_cell
SEXP first_cell(SEXP x);
RcppExport SEXP sourceCpp_1_first_cell(SEXP xSEXP) {
BEGIN_RCPP
    Rcpp::RObject __result;
    Rcpp::RNGScope __rngScope;
    Rcpp::traits::input_parameter< SEXP >::type x(xSEXP);
    __result = Rcpp::wrap(first_cell(x));
    return __result;
END_RCPP
}

Generated R functions 
-------------------------------------------------------

`.sourceCpp_1_DLLInfo` <- dyn.load('/private/var/folders/_1/4wg2xbj12_dft9p3kq4005n40000gn/T/RtmphBt39C/sourceCpp-x86_64-apple-darwin15.6.0-0.12.6/sourcecpp_16dd4b06d223/sourceCpp_2.so')

first_el <- Rcpp:::sourceCppFunction(function(x) {}, FALSE, `.sourceCpp_1_DLLInfo`, 'sourceCpp_1_first_el')
first_cell <- Rcpp:::sourceCppFunction(function(x) {}, FALSE, `.sourceCpp_1_DLLInfo`, 'sourceCpp_1_first_cell')

rm(`.sourceCpp_1_DLLInfo`)

Building shared library
--------------------------------------------------------

DIR: /private/var/folders/_1/4wg2xbj12_dft9p3kq4005n40000gn/T/RtmphBt39C/sourceCpp-x86_64-apple-darwin15.6.0-0.12.6/sourcecpp_16dd4b06d223

/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/bin/R CMD SHLIB -o 'sourceCpp_2.so'  'dispatch.cpp'  
clang++ -std=c++11 -I/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/include -DNDEBUG  -I/usr/local/opt/gettext/include -I/usr/local/opt/readline/include -I/usr/local/opt/openssl/include -I/usr/local/include  -I"/usr/local/lib/R/3.3/site-library/Rcpp/include" -I"/private/tmp"   -pedantic -Wall -fPIC  -g -O2 -c dispatch.cpp -o dispatch.o
clang++ -std=c++11 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/usr/local/opt/gettext/lib -L/usr/local/opt/readline/lib -L/usr/local/opt/openssl/lib -L/usr/local/lib -L/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/lib -L/usr/local/opt/gettext/lib -L/usr/local/opt/readline/lib -L/usr/local/opt/openssl/lib -L/usr/local/lib -o sourceCpp_2.so dispatch.o -F/usr/local/Cellar/r/3.3.1_3/R.framework/.. -framework R -lintl -Wl,-framework -Wl,CoreFoundation

And here is the result of compiling the same file under current master (0.12.7.5; many warnings):

> library(Rcpp)
> packageVersion('Rcpp')    
[1] '0.12.7.5'
> Rcpp::sourceCpp('/tmp/dispatch.cpp', verbose = TRUE)

Generated extern "C" functions 
--------------------------------------------------------


#include <Rcpp.h>
// first_el
SEXP first_el(SEXP x);
RcppExport SEXP sourceCpp_1_first_el(SEXP xSEXP) {
BEGIN_RCPP
    Rcpp::RObject rcpp_result_gen;
    Rcpp::RNGScope rcpp_rngScope_gen;
    Rcpp::traits::input_parameter< SEXP >::type x(xSEXP);
    rcpp_result_gen = Rcpp::wrap(first_el(x));
    return rcpp_result_gen;
END_RCPP
}
// first_cell
SEXP first_cell(SEXP x);
RcppExport SEXP sourceCpp_1_first_cell(SEXP xSEXP) {
BEGIN_RCPP
    Rcpp::RObject rcpp_result_gen;
    Rcpp::RNGScope rcpp_rngScope_gen;
    Rcpp::traits::input_parameter< SEXP >::type x(xSEXP);
    rcpp_result_gen = Rcpp::wrap(first_cell(x));
    return rcpp_result_gen;
END_RCPP
}

Generated R functions 
-------------------------------------------------------

`.sourceCpp_1_DLLInfo` <- dyn.load('/private/var/folders/_1/4wg2xbj12_dft9p3kq4005n40000gn/T/RtmpL5U0GG/sourceCpp-x86_64-apple-darwin15.6.0-0.12.7.5/sourcecpp_16e7a75501b20/sourceCpp_2.so')

first_el <- Rcpp:::sourceCppFunction(function(x) {}, FALSE, `.sourceCpp_1_DLLInfo`, 'sourceCpp_1_first_el')
first_cell <- Rcpp:::sourceCppFunction(function(x) {}, FALSE, `.sourceCpp_1_DLLInfo`, 'sourceCpp_1_first_cell')

rm(`.sourceCpp_1_DLLInfo`)

Building shared library
--------------------------------------------------------

DIR: /private/var/folders/_1/4wg2xbj12_dft9p3kq4005n40000gn/T/RtmpL5U0GG/sourceCpp-x86_64-apple-darwin15.6.0-0.12.7.5/sourcecpp_16e7a75501b20

/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/bin/R CMD SHLIB -o 'sourceCpp_2.so'  'dispatch.cpp'  
clang++ -std=c++11 -I/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/include -DNDEBUG  -I/usr/local/opt/gettext/include -I/usr/local/opt/readline/include -I/usr/local/opt/openssl/include -I/usr/local/include  -I"/usr/local/lib/R/3.3/site-library/Rcpp/include" -I"/private/tmp"   -pedantic -Wall -fPIC  -g -O2 -c dispatch.cpp -o dispatch.o
In file included from dispatch.cpp:1:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp.h:27:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/RcppCommon.h:29:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/r/headers.h:50:
In file included from /usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/macros.h:82:
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:57:42: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ___RCPP_RETURN___(_FUN_, _SEXP_, Vector, ##__VA_ARGS__)
                                         ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:36:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(INTSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:38:67: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(REALSXP, __FUN__, __TMP__, __RCPPTYPE__,            \
                                                                  ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:40:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(RAWSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:42:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(LGLSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:44:67: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(CPLXSXP, __FUN__, __TMP__, __RCPPTYPE__,            \
                                                                  ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:46:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(STRSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:48:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(VECSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:50:67: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(EXPRSXP, __FUN__, __TMP__, __RCPPTYPE__,            \
                                                                  ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    return ___FUN___(::Rcpp::___RCPPTYPE___<___RTYPE___>(___OBJECT___),        \
                                                                      ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:59:42: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ___RCPP_RETURN___(_FUN_, _SEXP_, Matrix, ##__VA_ARGS__)
                                         ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:36:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(INTSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:38:67: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(REALSXP, __FUN__, __TMP__, __RCPPTYPE__,            \
                                                                  ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:40:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(RAWSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:42:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(LGLSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:44:67: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(CPLXSXP, __FUN__, __TMP__, __RCPPTYPE__,            \
                                                                  ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:46:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(STRSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:48:66: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(VECSXP, __FUN__, __TMP__, __RCPPTYPE__,             \
                                                                 ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:50:67: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    ___RCPP_HANDLE_CASE___(EXPRSXP, __FUN__, __TMP__, __RCPPTYPE__,            \
                                                                  ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
    return ___FUN___(::Rcpp::___RCPPTYPE___<___RTYPE___>(___OBJECT___),        \
                                                                      ^
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/lib/R/3.3/site-library/Rcpp/include/Rcpp/macros/dispatch.h:30:71: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
34 warnings generated.
clang++ -std=c++11 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/usr/local/opt/gettext/lib -L/usr/local/opt/readline/lib -L/usr/local/opt/openssl/lib -L/usr/local/lib -L/usr/local/Cellar/r/3.3.1_3/R.framework/Resources/lib -L/usr/local/opt/gettext/lib -L/usr/local/opt/readline/lib -L/usr/local/opt/openssl/lib -L/usr/local/lib -o sourceCpp_2.so dispatch.o -F/usr/local/Cellar/r/3.3.1_3/R.framework/.. -framework R -lintl -Wl,-framework -Wl,CoreFoundation

I do wonder to what extent this PR is fixing a problem that doesn't exist (and creating new problems as an unfortunate side effect); AFAICT the only other use of the RCPP_RETURN_VECTOR macro (on public github, anyway) is in dplyr, and it's only used once there.

@eddelbuettel
Copy link
Member

Thanks for doing this.

And this is pretty much what I was wondering too:

I do wonder to what extent this PR is fixing a problem that doesn't exist (and creating new problems as an unfortunate side effect);

Regarding

AFAICT the only other use of the RCPP_RETURN_VECTOR macro (on public github, anyway) is in
dplyr, and it's only used once there.

that is a little harder to tell. By virtue of copy and paste some snippets appear in a lot of places -- 193 hits for a search GitHub.

Now: we have to go back and figure out what if anything actually the issue is here. If it is code that is rarely compiled in AND needs C++ AND is silent when we build and test AND (apparently) only rears his head when dplyr is built ... then I am not sure how much I am really willing to care... In a perfect world we would have time for all warts, in this world this issue may get a reasonably low score.

Other views?

@eddelbuettel
Copy link
Member

A somewhat closer look at those 193 hits confirms the hunch by @jayhesselberth -- seemingly a lot of packrat-induced copying of a single file from dplyr. Which, as I recall, is not compiling with C++11 so it may even get the other variant ...

@kevinushey
Copy link
Contributor

kevinushey commented Nov 3, 2016

You're right, token pasting of , and ##__VA_ARGS is indeed a GNU extension (although clang and gcc both implement it). I see the warnings with clang on my OS X box.

As an example of a CRAN package bitten by these warnings (note: the package doesn't depend on Rcpp): https://cran.r-project.org/web/checks/check_results_ctl.html

Nonetheless, this does imply that C++11-using packages that include Rcpp will potentially run afoul of this CRAN warning -- hence, we should try to fix it (avoid the use of token pasting here).

@eddelbuettel
Copy link
Member

One nasty short-term trick may be push and pop (just in this file) some pragma to suppress the warning.

@eddelbuettel
Copy link
Member

And just to put it out here: we can also revert this PR. I have no strong feelings either way.

@kevinushey
Copy link
Contributor

There's probably some workaround to ensure that we can use ##__VA_ARGS__ safely here -- http://stackoverflow.com/questions/32047685/variadic-macro-without-arguments seems to outline one such technique, so someone sufficiently brave (crazy?) enough to try could probably make it work ...

Barring that, I'd be in favor of reverting as well.

@jayhesselberth
Copy link

jayhesselberth commented Nov 3, 2016

I'm in favor of reverting, unless @artemklevtsov can work some variadic macro dark magic. Happy to test another version where e.g. this feature can be conditionally included and not generate warnings.

@artemklevtsov
Copy link
Contributor Author

Instead redefine the RCPP_RETURN_VECTOR with RCPP_USING_CXX11, we can add additional maco.

@eddelbuettel
Copy link
Member

eddelbuettel commented Nov 4, 2016

Yes, but that does not solve the "uses g++ extensions we should not use" ie __VA_ARGS__ issue.

eddelbuettel added a commit that referenced this pull request Nov 4, 2016
@eddelbuettel
Copy link
Member

@jayhesselberth Thanks again for the heads up. I pushed a new branch which goes back to the old file dispatch.h and should avoid these issues.

@artemklevtsov Sorry about this, and I should have caught this earlier. We are of course open to a reworked version, maybe with the trick from the SO post that @kevinushey found.

@nathan-russell
Copy link
Contributor

@eddelbuettel I think I have a patch for this, I just need to do some more extensive testing. If it ends up working properly, should I open a separate issue?

@eddelbuettel
Copy link
Member

@nathan-russell Nice -- you continue to be The Man!!

Whatever makes sense, and no rush. Just split it off master. In that sense my (defensive, suggested, reversing) branch may never become a PR.

I need to work on a talk the next couple of days but was thinking to aim the November release 0.12.8 for next weekend. So you have a couple of days. And if takes longer and slips into 0.12.9 no worries either. We get this cleaned up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arbitrary number of arguments in RCPP_RETURN_VECTOR
5 participants