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

Added constructor to instantiate newDatetimeVector from VectorBase. #577

Merged
merged 1 commit into from Nov 10, 2016
Merged

Added constructor to instantiate newDatetimeVector from VectorBase. #577

merged 1 commit into from Nov 10, 2016

Conversation

dcdillon
Copy link
Contributor

@dcdillon dcdillon commented Nov 5, 2016

This fixes the issue where

#include <Rcpp.h>


// [[Rcpp::export]]
Rcpp::newDatetimeVector test(Rcpp::newDatetimeVector v) {
    Rcpp::newDatetimeVector x = v + 2.;
        return x;
}

failed to compile.

It, however, is likely that other issues will crop up at some point due to other missing constructors (since Vector has so many and newDatetimeVector has so few). I'd like opinions on whether the rest of the Vector constructors should be ported to newDatetimeVector at this time, or if we should just wait and see what breaks and port them one at a time.

The same applies to operator=, as there don't exist any operator= functions that return newDateTimeVector &.

@eddelbuettel
Copy link
Member

Thank you. We probably want to
a) toss operator= in while we're at it, and
b) do the same for newDatevector.
If nobody gets to it I can do it while riding the train downstate next Thursday.

@dcdillon
Copy link
Contributor Author

dcdillon commented Nov 7, 2016

I'll add that stuff tonight.

@eddelbuettel
Copy link
Member

Maybe get newDatetimeVector right first, and then add things to newDatevector ?

@dcdillon
Copy link
Contributor Author

dcdillon commented Nov 7, 2016

Seems fair. The one thing that bothers me is that it's likely that some weird issues will bite us later on with missing ctors and assignments, but I also don't want to create a huge PR of largely untested code.

@eddelbuettel
Copy link
Member

What is your timeline for adding operator=()? Shall I merge now and we revisit extension later?

@dcdillon
Copy link
Contributor Author

Sorry. I ran into a weird issue on my machine and was getting check fails that I didn't understand and just haven't gotten back to it. Feel free to merge this and I'll get to the rest when I can

@eddelbuettel
Copy link
Member

Yes, let's do that.

@eddelbuettel eddelbuettel merged commit 2aeb676 into RcppCore:master Nov 10, 2016
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

2 participants