-
Notifications
You must be signed in to change notification settings - Fork 4
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
add xdr argument #6
Conversation
Love it. Saw the discussion on the list too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Left one minor comment.
Thinking it could do with a test. Should we add one? Maybe just extending tests/simpleTests.R (and its reference output) ?
DESCRIPTION
Outdated
@@ -1,8 +1,8 @@ | |||
Package: RApiSerialize | |||
Type: Package | |||
Title: R API Serialization | |||
Version: 0.1.2 | |||
Date: 2022-08-25 | |||
Version: 0.1.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this 0.1.2.1, please. I reserve 'a.b.c' versions for actual uploads I trigger.
src/serialize.cpp
Outdated
@@ -210,13 +211,23 @@ extern "C" SEXP serializeToRaw(SEXP object, SEXP versionSexp = R_NilValue) { | |||
version = Rf_asInteger(versionSexp); | |||
} | |||
if (version == NA_INTEGER || version <= 0) { | |||
Rf_error("bad version value"); | |||
Rf_error("bad version value"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the rest of the file uses four spaces indentation.
I will just make the changes. Also needs a new argument at the R caller. |
Ok, there were one or two little niggles missing I took care of (along with the one nag at CRAN about R_NO_REMAP). This should be ready for a new CRAN version now. Let me know your thoughts. |
Looks good! :) |
I shipped it to CRAN a few hours ago, but I suspect they may have an issue with their test runners as we should long have heard back from the few reverse depends. I guess we will hear by tomorrow. |
Booo. Was out for a nice bike ride and as I get back I am greeted by Uwe with a 'change to worse' for.... wait for it..... |
Fixed the blunder and re-uploaded. Because the existing package is tagged 'NoRemap' it requires a manual check which may happen early tomorrow during European morning hours. |
And as expected the package is now at CRAN. Yay. |
Hi Dirk, what do you think about adding an argument for choosing between XDR and binary formats? From my testing serialization using binary fromat is about 3.5x faster.
Comparison