Enhance Rcpp::Nullable::as with explicit cast (Closes #1470)#1471
Enhance Rcpp::Nullable::as with explicit cast (Closes #1470)#1471eddelbuettel wants to merge 4 commits intomasterfrom
Conversation
Enchufa2
left a comment
There was a problem hiding this comment.
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.
|
TBH I had forgotten these had and I just realize I still need to rename |
|
@enchufa You convinced me that PS Oh gnarl, may have to back that out or revisit as CI is unhappy. |
|
That's weird. Is it due to a different C++ standard with those versions? |
|
I honestly cannot work it out! But it is related to our You were right we want |
075dfc7 to
9714763
Compare
|
Ok rebased following merge and about to turn on the reverse-depends machine for the last currently-open PR. |
|
Ahh, sad news @Enchufa2 : If I remove the definition of the member function, it passes and builds. I could leave it behind an opt-in |
|
It also passes when I change 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 Paging @ctoney : Hi Chirs, I see you just released a |
|
How did that work before anyway? Shouldn't it get an error because there is no conversion from |
|
Don't ask me. It's on CRAN. Maybe implicitly via |
|
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. |
|
Awesome! As for
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 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 .... |
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 implicitas<>()needs a nudge, we can hide the nudge inside that (member function)as()function body accessing the matchingas<>for the given T. A little bit of ellbow grease.Checklist
R CMD checkstill passes all tests