Skip to content

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented May 7, 2018

These functions are basically string functions that are pretty long because they handle the general case. But since the calls use a lot of hard-coded strings, it's not necessary to copy the full thing, and the result is not too much longer.

I would have like to have used snprintf for extra safety (though there really shouldn't be any issue), but it's C99 and I'm not sure if you allow that.

@coveralls
Copy link

coveralls commented May 7, 2018

Coverage Status

Coverage decreased (-0.3%) to 78.101% when pulling 51e2a51 on QuLogic:less-git_buf-internals into 0eaa7fb on ropensci:master.

@QuLogic
Copy link
Contributor Author

QuLogic commented May 7, 2018

This leaves only the GIT_UNUSED macro and git__getenv internal functions, but the latter is a bit more complicated than these to be able to replace directly.

@stewid
Copy link
Member

stewid commented May 9, 2018

Great. I agree that snprintf is better and it shouldn't be an issue, so please go ahead and use that.
Can we add

#define GIT2R_UNUSED(x) ((void)(x))

and replace GIT_UNUSED?

Maybe there is something similar to git_getenv in the R C api, but I don't think so?

@QuLogic QuLogic force-pushed the less-git_buf-internals branch 2 times, most recently from be9e871 to 6e412f1 Compare May 12, 2018 07:37
QuLogic added 3 commits May 12, 2018 04:36
Signed-off-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Signed-off-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Signed-off-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@QuLogic QuLogic force-pushed the less-git_buf-internals branch from 6e412f1 to 51e2a51 Compare May 12, 2018 08:36
@QuLogic
Copy link
Contributor Author

QuLogic commented May 13, 2018

Updated to use snprintf.

@stewid stewid merged commit 91a169d into ropensci:master May 14, 2018
@stewid
Copy link
Member

stewid commented May 14, 2018

Thanks

@QuLogic QuLogic deleted the less-git_buf-internals branch May 15, 2018 00:42
@stewid
Copy link
Member

stewid commented May 15, 2018

I removed the use of the GIT_UNUSED macro

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.

3 participants