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

const support for Nullable<> #417

Merged
merged 5 commits into from
Jan 14, 2016
Merged

const support for Nullable<> #417

merged 5 commits into from
Jan 14, 2016

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Jan 13, 2016

cargo-culted from many other implementations of operator SEXP() const

This came up when trying to use const Nullable<...>& in https://github.com/rstats-db/RMySQL/pull/90/files#r49576837

Seemed too minor to open an issue first. What would a test for this look like?

CC @peternowee

krlmlr and others added 2 commits January 13, 2016 14:08
cargo-culted from many other implementations of `operator SEXP()`
@eddelbuettel
Copy link
Member

@dcdillon Care to look at this (and tests) ?

@dcdillon
Copy link
Contributor

I will take a peek soon. May not have time until the weekend (depends on the wife).

@@ -179,22 +179,22 @@ void test_stop_variadic() {
}

// [[Rcpp::export]]
bool testNullableForNull(Nullable<NumericMatrix> M) {
bool testNullableForNull(const Nullable<NumericMatrix>& M) {
Copy link
Member

Choose a reason for hiding this comment

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

Methinks we may want these as const variants (post PR) as well as non-const variants just to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's no big deal to re-add the pre-PR versions, but I wonder under what circumstances code can succeed for const& but fail for a non-const object?

@dcdillon
Copy link
Contributor

The question I have is with regards to the return type of SEXP of the get() function. It doesn't provide any safety...so what is the use case of a const Nullable<T> object?

@eddelbuettel
Copy link
Member

As for your motivation in https://github.com/rstats-db/RMySQL/pull/90/files#r49576837, why not just initialize with empty strings a la std::string foo = "". Why insinst on Nullable and require this change?

@dcdillon
Copy link
Contributor

What I would propose is adding an T as() function and a T as() const function to Nullable where the first one simply returns a T(m_sexp) and the second one returns a "make deep copy of SEXP and then create a T out of it" and return it. This way we get actual const safety out of a const Nullable<T>. It means that the efficiency of T as() const function would be really crappy, but I see no other way to make const Nullable<T> provide any safety at all.

@krlmlr
Copy link
Contributor Author

krlmlr commented Jan 13, 2016

The strings are passed to a C API which makes a difference between NULL and "". Rcpp::Nullable seemed handy.

@eddelbuettel
Copy link
Member

@dcdillon I like that. May look into what Rcpp::clone() does.

@krlmlr The idiom std::string foo = "hello, world"; const char* bar = foo.c_str(); is about as old asstd::string itself and gets you a C API string. Again, no motivation for Nullable yet.

@krlmlr
Copy link
Contributor Author

krlmlr commented Jan 13, 2016

In that C API, passing NULL means "get your data elsewhere", while passing "" means "use this datum, it is an empty string". How do I encapsulate that difference in a std::string?

@eddelbuettel
Copy link
Member

Right. std::string alone can't do that as it can't hold a NULL (pointer).

@eddelbuettel
Copy link
Member

Going to merge this as it should be of limited impact in its current form.

eddelbuettel added a commit that referenced this pull request Jan 14, 2016
const support for Nullable<>
@eddelbuettel eddelbuettel merged commit a8bbece into RcppCore:master Jan 14, 2016
@kevinushey
Copy link
Contributor

Also agree that the conversion operator should be const. 👍

@dcdillon
Copy link
Contributor

@kevinushey why should something that returns a value that allows you to modify the contents be const? I know it's done as a convenience across Rcpp, but it's bastardizing const pretty horribly. That is to say, if you can get a SEXP from a const function, the world is your oyster. This would seem to go against any "traditional" meaning of const.

@eddelbuettel
Copy link
Member

@dcdillon "This is R. Everything is bastardized."

We are also cheating somewhat with function interfaces like double foo(const & NumericVector x) because we cannot really undo the P in the SEXP. It is what it is, but we all feel it is better to adhere to, and converge to, standard idioms. As such, no harm in const qualifiers.

@krlmlr krlmlr deleted the patch-1 branch January 15, 2016 16:07
@dcdillon
Copy link
Contributor

Yeah...i was more just wondering the logic, not arguing with it.

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

4 participants