Skip to content

Enhance Rcpp::Nullable::as with explicit cast (Closes #1470)#1471

Open
eddelbuettel wants to merge 4 commits intomasterfrom
enhance/nullable
Open

Enhance Rcpp::Nullable::as with explicit cast (Closes #1470)#1471
eddelbuettel wants to merge 4 commits intomasterfrom
enhance/nullable

Conversation

@eddelbuettel
Copy link
Copy Markdown
Member

As discussed in #1470 and building on this morning's discussion at the bottom of #1451, we can make Nullable<T> a little nicer. And that is all this PR does: for cases where the implicit as<>() needs a nudge, we can hide the nudge inside that (member function) as() function body accessing the matching as<> for the given T. A little bit of ellbow grease.

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

Copy link
Copy Markdown
Member

@Enchufa2 Enchufa2 left a comment

Choose a reason for hiding this comment

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

Fine, although Nullable could be further improved. Having an as() method in the first place seems odd, because it deviates from the rest of the API. I.e., objects typically have T() and SEXP() operators. This one has the latter, and it would be nice to have the former for consistency instead of, or at least on top of, as(). Also typically set() and get() are private. See e.g. AttributeProxy, just to name something recent.

@eddelbuettel
Copy link
Copy Markdown
Member Author

eddelbuettel commented Apr 8, 2026

TBH I had forgotten these had as() and get() and I think they are barely used. The main use, and I just used it some more from another package, is just straight up instantiation as the T. I.e. in RQuantLib I let 'time to expiry' be either the double it always was (applied as 'time' relative to valuation date) or as a date and a number of functions now accomodate both types and then call this helper to determine the date:

QuantLib::Date getFutureDate(const QuantLib::Date today,
                             Rcpp::Nullable<double> maturity,
                             Rcpp::Nullable<QuantLib::Date> exDate) {
    if (exDate.isUsable()) {
        return Rcpp::as<QuantLib::Date>(exDate);
    } else if (maturity.isUsable()) {
        double mat = Rcpp::as<double>(maturity);
        // depending on the compile-time option, this is either intra-day or not
        // in minutes
        auto length = boost::posix_time::minutes(boost::uint64_t(mat * 360 * 24 * 60));
        return QuantLib::Date{today.dateTime() + length}; // high-res time ctos
    } else {
        Rcpp::stop("Excactly one of 'maturity' or 'exDate' needs to be supplied and be non-null.");
        return QuantLib::Date(); // not reached
    }
}

and I just realize I still need to rename today to evalDate or alike.

@eddelbuettel
Copy link
Copy Markdown
Member Author

eddelbuettel commented Apr 9, 2026

@enchufa You convinced me that operator T() is a good idea so I added that. But given that as() has been exposed for a decade I do not want to remove it overnight (also known as 'R Core-style' around here). I would be up for sticking deprecation warnings into as() if we think operator T() suits. As for get(), we could deprecate / hide at the same time.

PS Oh gnarl, may have to back that out or revisit as CI is unhappy.

@Enchufa2
Copy link
Copy Markdown
Member

Enchufa2 commented Apr 9, 2026

That's weird. Is it due to a different C++ standard with those versions?

@eddelbuettel
Copy link
Copy Markdown
Member Author

I honestly cannot work it out! But it is related to our Rcpp::String so it is a fair chance it is us. I am not sure I care.

You were right we want operator T. Back in the day we couldn't, apparently. Now we can, so now we do. Works for me.

@eddelbuettel
Copy link
Copy Markdown
Member Author

Ok rebased following merge and about to turn on the reverse-depends machine for the last currently-open PR.

@eddelbuettel
Copy link
Copy Markdown
Member Author

Ahh, sad news @Enchufa2 : operator T() is not going to make it. A regression in gdalraster:

gdalvector.cpp: In constructor ‘GDALVector::GDALVector(const Rcpp::CharacterVector&, const std::string&, bool, const Rcpp::Nullable<Rcpp::Vector<16> >&, const std::string&, const std::string&)’:
gdalvector.cpp:74:51: error: operands to ‘?:’ have different types ‘const Rcpp::Nullable<Rcpp::Vector<16> >’ and ‘Rcpp::Vector<16>’
   74 |           m_open_options(open_options.isNotNull() ?
      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~^
   75 |                          open_options : Rcpp::CharacterVector::create()),
      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gdalvector.cpp:74:51: note:   and each type can be converted to the other
make: *** [/usr/lib/R/etc/Makeconf:211: gdalvector.o] Error 1
ERROR: compilation failed for package ‘gdalraster’

If I remove the definition of the member function, it passes and builds.

I could leave it behind an opt-in #define with a comment. What do you think?

@eddelbuettel
Copy link
Copy Markdown
Member Author

It also passes when I change gdalraster by adding a cast

root@25dd6436899c:/tmp/gdalraster/gdalraster# diff -u src/gdalvector.cpp.orig src/gdalvector.cpp
--- src/gdalvector.cpp.orig     2026-04-10 18:38:13.351663913 +0000
+++ src/gdalvector.cpp  2026-04-10 18:35:23.322868566 +0000
@@ -72,7 +72,7 @@
                        const std::string &dialect = "")
         : m_layer_name(layer), m_dialect(dialect),
           m_open_options(open_options.isNotNull() ?
-                         open_options : Rcpp::CharacterVector::create()),
+                         (Rcpp::CharacterVector) open_options : Rcpp::CharacterVector::create()),
           m_spatial_filter(spatial_filter),
           m_ignored_fields(Rcpp::CharacterVector::create()),
           m_hDataset(nullptr), m_eAccess(GA_ReadOnly), m_hLayer(nullptr) {
root@25dd6436899c:/tmp/gdalraster/gdalraster# 

This is closer to the default pattern of Rcpp::Nullable<T>: when not null, for the given T we usually instantiate as T(obj).

Paging @ctoney : Hi Chirs, I see you just released a gdalraster two weeks ago. When do you plan a new release, and would you consider tossing a cast like the one I have in this comment into src/gdalvector.cpp ? (Context is a not overly-important small enhancement to Rcpp::Nullable which we are currently testing, and with about 1/3 of packages done only gdalraster croaked. No rush, we can wait at least until the reverse depends check is done.

@Enchufa2
Copy link
Copy Markdown
Member

How did that work before anyway? Shouldn't it get an error because there is no conversion from const Rcpp::Nullable<Rcpp::Vector<16>> to <Rcpp::Vector<16>?

@eddelbuettel
Copy link
Copy Markdown
Member Author

Don't ask me. It's on CRAN. Maybe implicitly via operator SEXP() ? C++ conversion rules are weird!

@ctoney
Copy link
Copy Markdown

ctoney commented Apr 10, 2026

I was actually planning another release pretty close to that one. There's a very small performance regression if progress bar is turned on which it is by default. A few feature adds with that make a release worthwhile. I was going to wait until about the one month mark just for CRAN etiquette but can do it whenever. I'll test that change on my end shortly but seems fine. Thanks for investigating it.

@eddelbuettel
Copy link
Copy Markdown
Member Author

eddelbuettel commented Apr 10, 2026

Awesome! As for

I was going to wait until about the one month mark just for CRAN etiquette

I usually wait a week to not trigger the nag and try to keep it under 'six in six months' which with RcppArmadillo and upstream's frequency had its challenges at times :) But I don't think you have to have a month-long gap. With CRANberries I see all sorts of patterns. Next day, same day, you name it.

As for context: I think by adding operator T() we are expanding the surface a little, and something that used to work for you no longer does, but as hinted above, it may not have been the cleanest anyway. But chapeau to using (test ? yes : no) for Nullable use -- that hadn't occured to until this week. But it also bit us here so maybe the old school if (obs.IsNotNull()) foo() else bar() is cleaner :)

In any event, we are probably not going to upload until July (unless CRAN forces us, you never know) so no rush. But I'd rather sort this out now as otherwise I will have forgotten about it by then ....

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.

3 participants