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.

Sign up for GitHub

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

Jump to bottom

[refactor] Reduce code duplication in c_api.cpp #3539

Merged
merged 3 commits into from
Nov 24, 2020
Merged

[refactor] Reduce code duplication in c_api.cpp #3539

merged 3 commits into from
Nov 24, 2020

Conversation

AlbertoEAF
Copy link
Contributor

Remove code duplication in c_api.cpp through use of templates.

@AlbertoEAF AlbertoEAF changed the title [refactor] c_api.cpp - Simplify code [refactor] Remove code duplication in c_api.cpp Nov 8, 2020
@AlbertoEAF AlbertoEAF changed the title [refactor] Remove code duplication in c_api.cpp [refactor] Reduce code duplication in c_api.cpp Nov 8, 2020
@AlbertoEAF
Copy link
Contributor Author

Can someone review? :)

@AlbertoEAF
Copy link
Contributor Author

I wonder if we should move the helper methods to a new file c_api_helpers.cpp + c_api_helpers.h and declare only the methods that we need from such helpers as the CSC_RowIterator and not the auxiliary methods to implement those such as the low level template methods.

AlbertoEAF
AlbertoEAF commented Nov 14, 2020
View reviewed changes
Copy link
Contributor Author

@AlbertoEAF AlbertoEAF left a comment

Choose a reason for hiding this comment

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

I wonder if we should add those 2 extra files (helpers apart from public API methods), or keep everything in the same file as it is - simpler, and less code.

src/c_api.cpp Show resolved Hide resolved
src/c_api.cpp Show resolved Hide resolved
@AlbertoEAF
Copy link
Contributor Author

I thought again and don't see splitting this into separate files as that useful.

@guolinke can you review?

@jameslamb
Copy link
Collaborator

Could you please update this PR to the latest master? We recently learned that our CI was missing one of the checks that replicates CRAN (#3585 ), and added a new R CI job (#3588). Sorry for the inconvenience.

guolinke
guolinke approved these changes Nov 24, 2020
View reviewed changes
Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

LGTM

@guolinke guolinke merged commit 5e24b80 into microsoft:master Nov 24, 2020
@AlbertoEAF AlbertoEAF deleted the refactor-c-api branch November 24, 2020 11:43
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@guolinke guolinke guolinke approved these changes

@btrotta btrotta Awaiting requested review from btrotta

@chivee chivee Awaiting requested review from chivee

Assignees
No one assigned
Labels
maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlbertoEAF @jameslamb @guolinke @StrikerRUS