Skip to content

const support for Nullable<> #417

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 14, 2016
Merged

const support for Nullable<> #417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbf104f
const support for Nullable
krlmlr Jan 13, 2016
324e75c
operator SEXP() is forwarded to get()
Jan 13, 2016
ff00bf7
use const& in Nullable tests
Jan 13, 2016
563d968
NEWS
Jan 13, 2016
2c57b98
Revert "operator SEXP() is forwarded to get()"
Jan 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions inst/NEWS.Rd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,17 @@
\newcommand{\ghpr}{\href{https://github.com/RcppCore/Rcpp/pull/#1}{##1}}
\newcommand{\ghit}{\href{https://github.com/RcppCore/Rcpp/issues/#1}{##1}}

\section{Changes in Rcpp version 0.12.4 (2016-01-XX)}{
\itemize{
\item Changes in Rcpp API:
\itemize{
\item \code{Nullable<>::operator SEXP()} and
\code{Nullable<>::get()} now also work for \code{const} objects
(PR \ghpr{417}).
}
}
}

\section{Changes in Rcpp version 0.12.3 (2016-01-10)}{
\itemize{
\item Changes in Rcpp API:
Expand Down
4 changes: 2 additions & 2 deletions inst/include/Rcpp/Nullable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ namespace Rcpp {
*
* @throw 'not initialized' if object has not been set
*/
inline operator SEXP() {
inline operator SEXP() const {
checkIfSet();
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove checkIfSet() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now forwarded to get().

Copy link
Member

Choose a reason for hiding this comment

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

I can see that. Let me rephrase: Why did you make that change? What is the reasoning?

I see no relation in that and the const change. So why mess with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it's an unrelated change. I included it, because it affects code in close proximity to the code that I was touching anyway. The reason for that change: in the unlikely event that the implementation of get() changes, you will also want to change the implementation of operator SEXP(), because they seem synonymous.

That said, I'm totally fine with excluding that particular change from this PR, if you prefer that.

return m_sexp;
}
Expand All @@ -86,7 +86,7 @@ namespace Rcpp {
*
* @throw 'not initialized' if object has not been set
*/
inline SEXP get() {
inline SEXP get() const {
checkIfSet();
return m_sexp;
}
Expand Down
8 changes: 4 additions & 4 deletions inst/unitTests/cpp/misc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

return M.isNull();
}

// [[Rcpp::export]]
bool testNullableForNotNull(Nullable<NumericMatrix> M) {
bool testNullableForNotNull(const Nullable<NumericMatrix>& M) {
return M.isNotNull();
}

// [[Rcpp::export]]
SEXP testNullableOperator(Nullable<NumericMatrix> M) {
SEXP testNullableOperator(const Nullable<NumericMatrix>& M) {
return M;
}

// [[Rcpp::export]]
SEXP testNullableGet(Nullable<NumericMatrix> M) {
SEXP testNullableGet(const Nullable<NumericMatrix>& M) {
return M.get();
}