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

Msgpack_read/write and some minor improvements #13

Merged
merged 2 commits into from Jun 21, 2018

Conversation

Projects
None yet
2 participants
@traversc
Contributor

traversc commented Jun 21, 2018

No description provided.

@@ -1,8 +1,8 @@
Package: RcppMsgPack
Type: Package
Title: 'MsgPack' C++ Header Files and Interface Functions for R
Version: 0.2.2
Date: 2018-05-06
Version: 0.2.3

This comment has been minimized.

@eddelbuettel

eddelbuettel Jun 21, 2018

Owner

I would call this 0.2.2.1 for the PR, and only go to 0.2.3 once we're ready for CRAN but no need to change it, I can do this here.

@@ -3,4 +3,5 @@ importFrom("Rcpp", "evalCpp")
export("msgpack_format", "msgpack_map", "msgpack_pack", "msgpack_simplify", "msgpack_unpack",
"msgpackFormat", "msgpackMap", "msgpackPack", "msgpackSimplify", "msgpackUnpack",
"msgpack_timestamp_encode", "msgpackTimestampEncode", "msgpack_timestamp_decode", "msgpackTimestampDecode",
"msgpack_write", "msgpackWrite", "msgpack_read", "msgpackRead",

This comment has been minimized.

@eddelbuettel

eddelbuettel Jun 21, 2018

Owner

I absolutely love our double convention :) Others should follow our lead ;-)

@@ -49,13 +49,13 @@ BEGIN_RCPP
END_RCPP
}
// c_timestamp_encode
RawVector c_timestamp_encode(double seconds, uint32_t nanoseconds);
RawVector c_timestamp_encode(double seconds, u_int32_t nanoseconds);

This comment has been minimized.

@eddelbuettel

eddelbuettel Jun 21, 2018

Owner

I would have thought uint32_t was more common / standard? I guess you like how u_ sticks out?

@eddelbuettel

Looks solid. I did not dive into the cpp file as GH did not show it, and it is a bit weird that the one test file shows it as a complete change. Oh well.

Will merge once travis is through.

@eddelbuettel eddelbuettel merged commit 9504c6e into eddelbuettel:master Jun 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@traversc

This comment has been minimized.

Contributor

traversc commented Jun 22, 2018

Pretty minor changes in C++. I'll summarize:

  • Use msgpack::sbuffer instead of std::stringstream as it allows direct access to the underlying data and seems to be marginally faster. Also use a template for the buffer (although probably incorrectly labeled "stream"... )

  • Use std::vector<SEXP> at first instead of List in the main c_unpack function, since you don't know ahead of time how many objects are in the message

  • got rid of all the global variables used for temporary assignment ... seems like bad practice

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Jun 22, 2018

Hm, trying to think if std::vector<SEXP> had bitten us before. Seems like an obvious idea but I can't recall if we did hit potholes.

I presume you had been running code like that your side for a while? Rest sounds cool -- nice work, once again!

@traversc

This comment has been minimized.

Contributor

traversc commented Jun 22, 2018

I haven't tested it extensively, but so far I haven't seen any problems. I'm guessing there might be an issue with scope/deallocation of the underlying data? Since I'm not returning or passing the vector around, perhaps we don't have to worry about that.

eddelbuettel added a commit that referenced this pull request Jun 29, 2018

small fixes to some g++-7 compiler warnings, rolled version
also updated NEWS and ChangeLog for #13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment