-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[R-package] use R standard routine to access read-only ints passed to C++ #4246
Conversation
/gha run r-solaris Workflow Solaris CRAN check has been triggered! 🚀 solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-d3e989e4aa394a59bb7795b6b2bb6718 |
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
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.
I think we can cast buffer_len
only once in two places below.
Co-authored-by: Nikita Titov <[email protected]>
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
nice! valgrind seems happy with that change (#4246 (comment)) and so does the rest of CI, so I'll merge this. |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
This is another step towards #3016. It proposed using R's standard interface to handle integers passed into the C++ side (for things like array size, buffer size, number of iterations, etc.).
Changes in this PR:
SEXP
type for any arguments passed from R to C++ that should be read-only integersRf_asInteger()
to convert those SEXP objects to ints (replacing LightGBM'sR_AS_INT
)lightgbm_R.h
andlightgbm_R.cpp
(e.g. parameter namednrow
in the header file andnum_row
in the implementation)Background
There is not a scalar integer type in R. When you run some code like
x <- 1L
, R creates a length-one array (referred to in R as a "vector").To treat such data as an integer, instead of an integer array, in C/C++, it's necessary to extract the first element of that array into a new
int
. R provides a convenience function,Rf_asInteger()
, for exactly that purpose.See https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Some-convenience-functions for more details.
Notes for Reviewers
This does not rely on #4242