Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUnit tests for readProtoFiles2 #45
Conversation
Add proto files within two projects with bidirectional dependencies
|
Nice work. I was a little put back when I saw a change to I'll let this sit here for a day or two so that @murraystokely and @jeroen can comment, but this looks good to me at first glance. Will look once more. Thanks for this! |
| @@ -101,6 +101,12 @@ readProtoFiles2 <- function(files, | |||
| invisible(NULL) | |||
| } | |||
|
|
|||
| resetDescriptorPool <- function(){ | |||
| .Call( "resetDescriptorPool_cpp" ) | |||
eddelbuettel
May 21, 2018
Owner
I guess .Call() used to want a PACKAGE= argument but under registration=TRUE that should not be needed. I do wonder if we should purge the .Call() for new code and eg here just call resetDescriptorPool_cpp() ...
I guess .Call() used to want a PACKAGE= argument but under registration=TRUE that should not be needed. I do wonder if we should purge the .Call() for new code and eg here just call resetDescriptorPool_cpp() ...
siddharthab
May 21, 2018
Author
Contributor
I added the PACKAGE argument. I think using the registered symbols instead of a string should be part of a package wide refactoring. In my understanding, we still have to use .Call; it's just that we provide a symbol instead of expecting .Call to search for the name.
I added the PACKAGE argument. I think using the registered symbols instead of a string should be part of a package wide refactoring. In my understanding, we still have to use .Call; it's just that we provide a symbol instead of expecting .Call to search for the name.
| @@ -102,7 +102,7 @@ readProtoFiles2 <- function(files, | |||
| } | |||
|
|
|||
| resetDescriptorPool <- function(){ | |||
| .Call( "resetDescriptorPool_cpp" ) | |||
| .Call( "resetDescriptorPool_cpp", PACKAGE = "RProtoBuf" ) | |||
eddelbuettel
May 21, 2018
Owner
I was about to say we don't need this -- but then I realized that this package is still old-school not using Rcpp Attributes. Adding this follow the code convention even though we now also register symbols so I think it is not strictly needed -- but won't harm or hurt.
I was about to say we don't need this -- but then I realized that this package is still old-school not using Rcpp Attributes. Adding this follow the code convention even though we now also register symbols so I think it is not strictly needed -- but won't harm or hurt.
eddelbuettel
merged commit 3798b31
into
eddelbuettel:master
|
Thanks again! |
As requested in #39 and discussed in #40.