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

Avoid copies in String #1219

Merged
merged 3 commits into from
May 26, 2022
Merged

Conversation

p00ya
Copy link
Contributor

@p00ya p00ya commented May 23, 2022

Add move constructor/assignment for String&& and std::string&&, and
attempt to preserve the buffer/SEXP representation when copying in the
existing copy ctor and assignment operators.

Closes #1218.

Checklist

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

Add move constructor/assignment for String&& and std::string&&, and
attempt to preserve the buffer/SEXP representation when copying in the
existing copy ctor and assignment operators.

Closes RcppCore#1218.
@eddelbuettel
Copy link
Member

Looks very promising!

@@ -1,4 +1,4 @@
2018-10-25 Dean Scarff <dos@scarff.id.au>
2022-05-23 Dean Scarff <dos@scarff.id.au>
Copy link
Member

Choose a reason for hiding this comment

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

Hah! 😉

@eddelbuettel
Copy link
Member

Reverse-dependency checks are running, slowly, on an older machine. No compilation or issues so far (besides "mechanical" stuff a few 'new-since-last-check' packages need additional build depends involved, and one graphics-device package needs a rebuild for R 4.2.0) so the PR is looking good from that angle.

@Enchufa2
Copy link
Member

Looks good on first (very quick) pass. I can go over it more carefully in a few days if needed.

@p00ya
Copy link
Contributor Author

p00ya commented May 24, 2022

One thing for reviewers to pay attention to: the new unit tests need C++11 and I didn't add guards in there. I'm a bit confused about what the expected C++98 support is in Rcpp now: eddelbuettel@ said Rcpp has "C++11 as a baseline" in #1218, but Contributing.md says C++11 code needs guards. The inst/include stuff should be fine with C++98 (though admittedly I haven't tested this unless it's covered by R CMD check), but the tests would fail.

@eddelbuettel
Copy link
Member

If no language level is specified R uses its default which for R 4.2.0 (and 4.1.0 onwards actually) is C++14, R 4.0.0 brought us eleven:

1.2.4 Using C++ code

R can be built without a C++ compiler although one is available (but not
necessarily installed) on all known R platforms. As from R 4.0.0 a C++
compiler will be selected only if it conforms to the 2011 standard
(‘C++11’). A minor update(1) (‘C++14’) was published in December 2014
and will be used by default as from R 4.1.0 if supported. Further
revisions ‘C++17’ (in December 2017), and ‘C++20’ (with many new
features in December 2020) have been published since.

So we should be good but your point is well taken: these tests could fail for someone running them under R 3.6.3, say.

I am not sure if we need (or want) to worry about that.

@eddelbuettel
Copy link
Member

eddelbuettel commented May 25, 2022

That said, we have battled C++(11) availability for so long that we do of course have tools:

> Rcpp:::capabilities()
    variadic templates      initializer lists     exception handling     tr1 unordered maps 
                 FALSE                   TRUE                   TRUE                   TRUE 
    tr1 unordered sets           Rcpp modules             demangling            classic api 
                  TRUE                   TRUE                   TRUE                  FALSE 
             long long   C++0x unordered maps   C++0x unordered sets     Full C++11 support 
                  TRUE                   TRUE                   TRUE                   TRUE 
new date(time) vectors 
                  TRUE 
> Rcpp:::capabilities()[["Full C++11 support"]]
[1] TRUE
> 

So maybe it would be best to wrap that test around this, or call exit_file("Skipping for lack of C++11") if you see FALSE.

PS I knew there was one more. I can't remember why we never wrapped this from R but there is

> .Call(Rcpp:::rcpp_can_use_cxx11)
[1] TRUE
> 

which at least is a bit shorter.

@eddelbuettel
Copy link
Member

Ok, the reverse depends ran and have no issues whatsoever (commit of summary is here as usual). And I just added a baby commit to this PR to wrap an if (...C++11...) around the new tests, using the previously-discussed predicate which is used in another test.

So I think we're good here. Comments, if any? I may wait a day as there is no rush and then merge this tomorrow.

@eddelbuettel
Copy link
Member

No further issues so happily merging what looks like a pretty nice PR -- thanks again!

@eddelbuettel eddelbuettel merged commit e6ce1d8 into RcppCore:master May 26, 2022
eddelbuettel added a commit that referenced this pull request May 26, 2022
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.

[FR] Move semantics for Rcpp::String
3 participants