Skip to content
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] Move more functions to array interface #9824

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

david-cortes
Copy link
Contributor

ref #9810
From comment #9734 (comment)

This PR moves more R functions to the new array interface.

Here I've created a copy-paste of the matrix function for 1d vectors. I'm not sure why it needs [[nodiscard]] qualifiers though.

Along the way, it also solves a potential memory leak in the previous PR in which the call to CHECK_CALL could potentially lead to leaked memory of std::string objects that are used for the array interface.

@trivialfis
Copy link
Member

I'm not sure why it needs [[nodiscard]] qualifiers though.

Not required, it's just a suggestion from clang tidy to annotate functions with nodiscard when possible. We currently disable the suggestion to avoid massive change, but eventually we will turn it on. Feel free to ignore it if it's not useful.

potentially lead to leaked memory of std::string

That's difficult to get right. I think I will forget about it after a period of time without writing R c code. Is it possible to return errors in a more linear fashion?

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work on using array interface!

@trivialfis trivialfis merged commit 95af5c0 into dmlc:master Nov 30, 2023
26 checks passed
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.

2 participants