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

Nullable object (closes #363) #368

Merged
merged 6 commits into from
Sep 8, 2015
Merged

Conversation

eddelbuettel
Copy link
Member

This (simple) PR has been tested in pending code that will may go into mvabund in a few days. (I may use a copy there to not have to rely on the very newest Rcpp there.) But as the feature is generally useful (and I also already spotted use cases for it in RcppDE).

The basic need was described in #363, and we had a lively discussion there. @dcdillon and went back and forth over some design with a templated class but this doesn't quite work: the is circular as we need a NULL protection layer around our proxy objects. As soon as we template with them, access when NULL leads to an instantiation which ... throws as before. Similarly, I got nowhere with the safe_bool_idiom approach.

So what is here works, but (as always) could be improved if someone is so inclined. I added a few tests to describe how it is used.

@dcdillon
Copy link
Contributor

dcdillon commented Sep 7, 2015

Given that isNull() throws if it has never been set, should you also add an isSet() function? This would give the user the ability to poll for the "third state" without resorting to exception handling.

@eddelbuettel
Copy link
Member Author

You could you be "not set" but "not null" ?

Wait. I think I get it now. That is a good idea. Adding ...

* @throw 'not initialized' if object has not been set
*/
inline operator SEXP() {
isSet();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be checkIfSet()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely. Now fixed.

I blame @dcdillon for enticing me to make changes, which I then changed midflight... ;-)

@kevinushey
Copy link
Contributor

My only thought: the class name Nullable does not really evoke the type of the underlying object; e.g. I would much rather see Nullable<SEXP> to indicate that this is an object wrapping a SEXP that may be either unset or R_NilValue. It would be even better if this were a template class, e.g. Nullable<T> where T could be any Rcpp class type.

EDIT: Just saw your comment re: why you didn't go that route... it's a shame; I wonder if there's some other fix.

I'm still not quite sure on the utility -- can you provide some other examples where this would be used? (The example in #363 was a bit bare bones; a full example would be useful)

* Test function to check if object has been initialized
*
*/
inline bool isSet(void) { return m_set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably don't need void in the parameter list

@eddelbuettel
Copy link
Member Author

Nullable<SEXP> could work whereas Nullable<RcppANYTHING> does not seem to work given how our types are set up -- they bark at run-time when you inquire about their state and the payload was NULL.

One use case is in this PR where I carried a local copy over. We can now do this in a compact way whereas the current code is much uglier -- and exposes a SEXP in the nice Rcpp Attributes interface.

Suggestions for finer tuning welcome. What is there now is so far good enough for me.

@kevinushey
Copy link
Contributor

Okay -- LGTM!

@eddelbuettel
Copy link
Member Author

Thanks. @dcdillon and I went back and forth on this. We both really wanted Nullable<T> but you just instrument it as Nullable<NumericMatrix> and then pass NULL at run-time. It gets you back to NULL being used to instantiate NumericMatrix :-/

Maybe it can still be done -- suggestions welcome. We now have a few units describing what this should be good for, and one test use over in [RcppGSL] -- and those may help provide a framework for something better. But for now I'd say let's put this one too.

@eddelbuettel
Copy link
Member Author

Ok, being stubborn helps sometimes.

We had another crack at this and @dcdillon spotted a really bad NULL that really wanted to be a R_NilValue plus a few more little things -- so we WILL have a templated version coming.

eddelbuettel added a commit that referenced this pull request Sep 8, 2015
@eddelbuettel eddelbuettel merged commit c3686a2 into master Sep 8, 2015
@eddelbuettel eddelbuettel deleted the feature/nullable_issue363 branch September 9, 2015 19:38
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

3 participants