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

Fixing ambiguous arithmetic operators for Matrix/Vector on 32-bit #476

Merged
merged 3 commits into from
May 15, 2016
Merged

Fixing ambiguous arithmetic operators for Matrix/Vector on 32-bit #476

merged 3 commits into from
May 15, 2016

Conversation

dcdillon
Copy link
Contributor

So the crux of this is it makes templates that match exactly the function calls that happened. I achieved this by adding another template parameter to each of the arithmetic operators for for Vector and Matrix and then restricting the values allowed for this template parameter using enable_if.

Additionally, the same ambiguities can be exposed with the following C++ code on 64-bit (though it's unlikely anyone would have done this):

#include <Rcpp.h>

void testFunction(Rcpp::NumericVector x, long l)
{
        Rcpp::NumericVector y = x + l;
}

void testFunction2(Rcpp::NumericMatrix x, long l)
{
        Rcpp::NumericMatrix y = x + l;
}

@eddelbuettel, @kevinushey, @thirdwing please have a look.

@dcdillon
Copy link
Contributor Author

Hmm...perhaps it should be traits::is_convertible<T, typename traits::storage_type<RTYPE>::type> instead of traits::is_primitive.

Will do that if people agree.

…& to be more C++'y, although it hardly matters as all the types in question fit into a register
@thirdwing
Copy link
Member

Just confirmed this fixed 64-bit and 32-bit situations.

Let's merge this if it doesn't break any other packages.

@eddelbuettel
Copy link
Member

We just got Rcpp 0.12.5 onto CRAN so we have a moment -- but yes, will rev.dep this.

@eddelbuettel
Copy link
Member

With the various 'LGTM' having come in I'll just merge, turn on a rev.dep run -- and have the option to revert later today should it be necessary.

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.

None yet

3 participants