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

More thorough use of storage policy in Vector internals #850

Merged

Conversation

@romainfrancois
Copy link
Contributor

@romainfrancois romainfrancois commented May 4, 2018

This was a but more than I anticipated. Probably not exhaustive. but it makes the tests pass.

// [[Rcpp::export]]
int CharacterVectorNoProtect(Vector<STRSXP, NoProtectStorage> s){
    s[0] = "";
    return s.size();
}

// [[Rcpp::export]]
CharacterVector CharacterVectorNoProtect_crosspolicy(Vector<STRSXP, NoProtectStorage> s){
    CharacterVector s2(1) ;
    s2[0] = s[0];
    return s;
}

// [[Rcpp::export]]
List ListNoProtect_crosspolicy(Vector<VECSXP, NoProtectStorage> data){
    List data2(1) ;
    data2[0] = data[0];
    return data2;
}
@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented May 4, 2018

I see this failing on Travis. At least this test has an issue:

1 Test Suite : 
Rcpp Unit Tests - 589 test functions, 0 errors, 1 failure
FAILURE in test.DataFrame.CreateTwo.stringsAsFactors: Error in checkEquals(createTwoStringsAsFactors(), DF, msg = "DataFrame create2 stringsAsFactors = false") : 
  Names: 2 string mismatches
DataFrame create2 stringsAsFactors = false

Can you take another look?

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented May 4, 2018

Looks like a name attribute may not get dropped ?

R> library(Rcpp)
R> sourceCpp("~/git/rcpp/inst/unitTests/cpp/DataFrame.cpp")
R>         DF <- data.frame(a=1:3, b=c("a","b","c"), stringsAsFactors = FALSE )
R> RUnit::checkEquals( createTwoStringsAsFactors(), DF, msg = "DataFrame create2 stringsAsFactors = false")
Error in RUnit::checkEquals(createTwoStringsAsFactors(), DF, msg = "DataFrame create2 stringsAsFactors = false") : 
  Names: 2 string mismatches
DataFrame create2 stringsAsFactors = false
R> DF
  a b
1 1 a
2 2 b
3 3 c
R> createTwoStringsAsFactors()
  X1.3 c..a....b....c..
1    1                a
2    2                b
3    3                c
R> 
@romainfrancois
Copy link
Contributor Author

@romainfrancois romainfrancois commented May 4, 2018

should be better now.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented May 4, 2018

Yes, indeed -- thanks.

I happened to have run reverse depends on a machine this week, so I turned on a new one. We'll know in a few hours if that shows something unexpected.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented May 4, 2018

No issues, so merging.

@eddelbuettel eddelbuettel merged commit 89e6d83 into RcppCore:master May 4, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eddelbuettel added a commit to RcppCore/rcpp-logs that referenced this pull request May 4, 2018
@romainfrancois
Copy link
Contributor Author

@romainfrancois romainfrancois commented May 4, 2018

Great thanks. I’ll probably find out more of these if I start using alternative storage policies.

@romainfrancois romainfrancois deleted the romainfrancois:NoProtectStorage_string_proxies branch May 4, 2018
@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented May 4, 2018

BTW I have had dplyr bomb for the last little while but I guess you guys know that anyway as it currently bombs at CRAN too ...

See eg here for a summary. I have logs if you need them.

@romainfrancois
Copy link
Contributor Author

@romainfrancois romainfrancois commented May 5, 2018

A new version has been sent to cran, two weeks ago.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented May 5, 2018

Right. I even knew that. RcppArmadillo sat in the same 'inspect' corner for over a week too.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.