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

Error when passing arguments by reference #278

Closed
kafku opened this issue Mar 16, 2015 · 12 comments · Fixed by #400
Closed

Error when passing arguments by reference #278

kafku opened this issue Mar 16, 2015 · 12 comments · Fixed by #400

Comments

@kafku
Copy link
Contributor

kafku commented Mar 16, 2015

I got compilation errors when I compiled the following code using Rcpp::sourceCpp.

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
NumericMatrix test(const NumericMatrix &data){
  return data;
}

The error messages said,

error: invalid initialization of non-const reference of type ‘Rcpp::ConstInputParameter<Rcpp::Matrix<14, Rcpp::PreserveStorage> >&’ from a temporary of type ‘SEXPREC*
error: invalid conversion from ‘Rcpp::ConstInputParameter<Rcpp::Matrix<14, Rcpp::PreserveStorage> >
’ to ‘int’
error: initializing argument 1 of ‘Rcpp::Matrix<RTYPE, StoragePolicy>::Matrix(const int&) [with int RTYPE = 14, StoragePolicy = Rcpp::PreserveStorage]’

Then, I checked RcppExports.cpp generated by Rcpp::compileAttributes, which was like as follows.

// This file was generated by Rcpp::compileAttributes
// Generator token: 10BE3573-1514-4C36-9D1C-5A225CD40393

#include <Rcpp.h>

using namespace Rcpp;

// test
NumericMatrix test(const NumericMatrix &data);
RcppExport SEXP test_pkg_test(SEXP &dataSEXP) {
BEGIN_RCPP
    Rcpp::RObject __result;
    Rcpp::RNGScope __rngScope;
    Rcpp::traits::input_parameter< const NumericMatrix >::type &data(&dataSEXP);
    __result = Rcpp::wrap(test(&data));
    return __result;
END_RCPP
}

It seems that the above code

   Rcpp::traits::input_parameter< const NumericMatrix >::type &data(&dataSEXP);
    __result = Rcpp::wrap(test(&data));

should be

    Rcpp::traits::input_parameter< const NumericMatrix &>::type data(dataSEXP);
    __result = Rcpp::wrap(test(data));

This problem did not occur in Rcpp(0.11.3)
Here is the information about my environment

  • centos6.5
  • gcc 4.4.7
  • R 3.1.0
  • Rcpp 0.11.5.1

Thank you.

@eddelbuettel
Copy link
Member

Thanks for the report -- confirmed under Ubuntu 14.10 with g++ 4.9.2.

Removing the & is all it takes but it would be nice if this passed. For what it is worth, it also passes for const NumericVector &d.

@eddelbuettel
Copy link
Member

Also, it works with RcppArmadillo matrices:

#include <RcppArmadillo.h>

// [[Rcpp::depends(RcppArmadillo)]]

// [[Rcpp::export]]
arma::mat test(const arma::mat & d){
  return d;
}

@jjallaire
Copy link
Member

This is actually not a new bug (it's also in 0.11.3). This issue comes from the placement of the reference designator (&) next to the variable rather than the type. If you do this it will work:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
NumericMatrix test(const NumericMatrix & data){
  return data;
}

@sanmai-NL
Copy link

I have this problem still. The code reformatting of my IDE CLion (with its default settings) had moved the & around in function signatures, resulting in this compilation error. Should this issue not receive more attention?

@eddelbuettel
Copy link
Member

We are open to pull requests. We do not use CLion as an IDE so it is not exactly our problem...

@kevinushey
Copy link
Contributor

Evidence that everyone who binds * or & to variable names, instead of the types, deserves what they get ;)

Joking aside, this is genuinely a bug (I'm surprised nobody else has bumped into it thus far) but I would just ask CLion to change where it puts those tokens when reformatting.

@Thell
Copy link

Thell commented Nov 26, 2015

Just hit this after using clang-format (style=LLVM) which attaches the reference operator on the param name side. At first I thought WTF! Then after seeing the generated RcppExports.cpp a simple addition of --style="{BasedOnStyle: llvm, PointerAlignment: Left}" fixed it. Makes me wonder if clang could be used as the parser in the first place...

@eddelbuettel
Copy link
Member

Ohhhh now you're talking. Volunteering? ;-)

@eddelbuettel
Copy link
Member

We would of course carry the whole clang infrastructure with us, which ain't lightweight. RStudio has it (hi, @kevinushey) to parse for autocomplete and whatnot but we don't want to make behaviour of Rcpp depend on what IDE you use.

@dcdillon
Copy link
Contributor

I'm not sure what libclang does with comments (which is necessary for the // [[Rcpp::export]] type stuff. Just a thought.

@Thell
Copy link

Thell commented Nov 26, 2015

I think clang's libformat (the backend of clang-format) gives access to the high level AST which , I'd guess, includes raw comments, and they can definitely be parsed (clang-format itself does this).

But, I wasn't really serious. More like a brain murmer that made it to the keyboard. 😃 It would be fantastic though...

@jjallaire
Copy link
Member

One way to use libclang would be to search for it on the system and use it only when it's found. RStudio actually has a libclang wrapper which links dynamically at runtime rather than statically at build time so this wouldn't create a hard dependency on libclang (which we couldn't really do given CRAN). You can see how we do this here: https://github.com/rstudio/rstudio/blob/master/src/cpp/core/libclang/LibClang.cpp

So we could have two parsing implementations, one of which works when libclang is installed and then the fallback which we have now. The main benefit of this would be the ability to handle namespaced types and typedefs as well as not ever running into problems like this one.

eddelbuettel added a commit that referenced this issue Nov 27, 2015
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 a pull request may close this issue.

7 participants