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 as() and clone() to Nullable #423

Merged
merged 5 commits into from
Jan 20, 2016
Merged

Added as() and clone() to Nullable #423

merged 5 commits into from
Jan 20, 2016

Conversation

dcdillon
Copy link
Contributor

Fixes #421

@eddelbuettel
Copy link
Member

LGTM

/**
* Return a clone of m_sexp as a T
*/
inline T clone() const { return Rcpp::clone(Rcpp::as< T >(get())); }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So technically the call is just AWFUL it:

  • Shallow copies in the return from Rcpp::as()
  • Deep copies inside the `Rcpp::clone()' function
  • Shallow copies in the return from Rcpp::clone()
  • Shallow copies in the return from Nullable::clone()
  • Probably shallow copies again when the end user receives it.

There should really be a better way. Best I can come up with is a SEXP Rcpp::clone(SEXP obj) function which would remove the need for some of this.

Any other thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we usually call wrap() to convert to a SEXP which would make feeding in clone() more straightfoward?

@dcdillon
Copy link
Contributor Author

OK. It's a lot better now. We keep it as a SEXP until return time. So now it's down to:

  • Construct T
  • Copy in return

Much more satisfying.

@dcdillon
Copy link
Contributor Author

And same performance tweak to as(). Now I think it's good.

@eddelbuettel
Copy link
Member

Well, nobody cared to second this but on the upside nobody howled either :)

Planning to fold this in later this morning.

@dcdillon
Copy link
Contributor Author

No problem. It's just convenience stuff.

@dcdillon
Copy link
Contributor Author

Added isUsable() to indicate whether or not the Nullable<T> can be used as a T. This is equivalent to checking isSet() followed by !isNull() but won't throw an exception.

@eddelbuettel
Copy link
Member

No one shouted so will fold this one now.

eddelbuettel added a commit that referenced this pull request Jan 20, 2016
Added as() and clone() to Nullable
@eddelbuettel eddelbuettel merged commit 05b11bf into RcppCore:master Jan 20, 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.

2 participants