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

Range sugar uses R_xlen_t as start/end type #568

Merged
merged 1 commit into from
Oct 24, 2016

Conversation

thirdwing
Copy link
Member

No description provided.

@eddelbuettel
Copy link
Member

Looks (once again) good to me -- thanks you!

@eddelbuettel
Copy link
Member

Any other votes on this? Don't make me feel so BDFL by merging without seconds ...

@kevinushey
Copy link
Contributor

LGTM!

@eddelbuettel eddelbuettel merged commit 3348462 into RcppCore:master Oct 24, 2016
@thirdwing thirdwing deleted the iss391_patch3 branch October 24, 2016 19:55
@eddelbuettel
Copy link
Member

One possible issue. I just concluded another full rev. dep (and just committed log here), and the zeros/ones/eye issue is now squeaky clean.

But we have one new issue:

boot_time_inference.cpp: In function ‘Rcpp::IntegerVector cbbSequence(int, int)’:
boot_time_inference.cpp:382:52: error: ambiguous overload for ‘operator-’ (operand types are ‘Rcpp::Range’ and ‘int’)
     IntegerVector to_write = Range((j-1)*b + 1,j*b)-1;
                                                    ^
In file included from /usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/sugar_forward.h:31:0,
                 from /usr/local/lib/R/site-library/Rcpp/include/RcppCommon.h:175,
                 from /tmp/RcppDepends/lib/RcppArmadillo/include/RcppArmadilloForward.h:26,
                 from /tmp/RcppDepends/lib/RcppArmadillo/include/RcppArmadillo.h:31,
                 from /tmp/RcppDepends/lib/RcppArmadillo/include/RcppArmadilloExtensions/fixprob.h:25,
                 from /tmp/RcppDepends/lib/RcppArmadillo/include/RcppArmadilloExtensions/sample.h:30,
                 from boot_time_inference.cpp:2:
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/Range.h:77:15: note: candidate: Rcpp::Range Rcpp::Range::operator-(R_xlen_t)
         Range operator-( R_xlen_t n ){
               ^
In file included from /usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/operators/operators.h:32:0,
                 from /usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/sugar.h:30,
                 from /usr/local/lib/R/site-library/Rcpp/include/Rcpp.h:71,
                 from /tmp/RcppDepends/lib/RcppArmadillo/include/RcppArmadillo.h:34,
                 from /tmp/RcppDepends/lib/RcppArmadillo/include/RcppArmadilloExtensions/fixprob.h:25,
                 from /tmp/RcppDepends/lib/RcppArmadillo/include/RcppArmadilloExtensions/sample.h:30,
                 from boot_time_inference.cpp:2:
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/operators/minus.h:417:1: note: candidate: typename Rcpp::traits::enable_if<Rcpp::traits::is_convertible<typename Rcpp::traits::remove_const_and_reference<U>::type, typename Rcpp::traits::storage_type<RTYPE>::type>::value, Rcpp::sugar::Minus_Vector_Primitive<RTYPE, NA, T> >::type Rcpp::operator-(const Rcpp::VectorBase<RTYPE, NA, VECTOR>&, const U&) [with int RTYPE = 13; bool NA = false; T = Rcpp::Range; U = int; typename Rcpp::traits::enable_if<Rcpp::traits::is_convertible<typename Rcpp::traits::remove_const_and_reference<U>::type, typename Rcpp::traits::storage_type<RTYPE>::type>::value, Rcpp::sugar::Minus_Vector_Primitive<RTYPE, NA, T> >::type = Rcpp::sugar::Minus_Vector_Primitive<13, false, Rcpp::Range>]
 operator-(
 ^
/usr/lib/R/etc/Makeconf:141: recipe for target 'boot_time_inference.o' failed
make: *** [boot_time_inference.o] Error 1
ERROR: compilation failed for package ‘bootTimeInference’

@thirdwing Could you take a look? And if you do please upgrade RcppArmadillo to the GH master (as that version suppresses another noisy 'deprecated' warning from Conrad).

CCing @coatless @kevinushey

@thirdwing
Copy link
Member Author

Will look into this.

@thirdwing
Copy link
Member Author

My mistake. I shouldn't change int into R_xlen_t in https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/sugar/Range.h#L63-L77

This now causes some ambiguousity.

@eddelbuettel
Copy link
Member

eddelbuettel commented Oct 25, 2016

You're fast as always, and not to worry -- that is why we run these integration tests.

I guess we could have two sets--for R_xlen_t and for int ?

@thirdwing
Copy link
Member Author

I will fix this and add some more testing. The failing code can be good unit test.

eddelbuettel added a commit that referenced this pull request Oct 26, 2016
fix range sugar and add unit test (fix error report in #568)
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.

3 participants