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

[SWIG] add size_t manipulation functions #3895

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

shuttie
Copy link
Contributor

@shuttie shuttie commented Feb 2, 2021

I'm a maintainer of the lightgbm4j library, and would like to expose the LGBM_DatasetGetFeatureNames function in a more java-friendly way.

The problem with the LGBM_DatasetGetFeatureNames function is that it requires a size_t type param, which is not possible to instantiate at all, as SWIG definition file only exposes int/long/double/float/int64_t/int32_t types.

This one-liner PR also exposes a size_t function family in SWIG wrapper, so lightgbmlibJNI.java will also include these new ones:

>   public final static native long new_size_tp();
>   public final static native long copy_size_tp(long jarg1);
>   public final static native void delete_size_tp(long jarg1);
>   public final static native void size_tp_assign(long jarg1, long jarg2);
>   public final static native long size_tp_value(long jarg1);

@jameslamb
Copy link
Collaborator

Thanks for contributing!

@AlbertoEAF or @imatiach-msft , could you comment on this?

@AlbertoEAF
Copy link
Contributor

Hello @shuttie :)

Thanks for contributing.There are already several LightGBM Java providers out there which are using such functions in Java through a int64_t when size_t pointers are needed. However, I agree that is more correct to use a proper size_t pointer.

LGTM ;)

@shuttie
Copy link
Contributor Author

shuttie commented Feb 3, 2021

Using int64_t as a size_t can break code in a case if someone will try to build this library on 32-bit systems (like an Android phone not on Aarch64): AFAIK size_t is defined as unsigned int on 32-bit ARM.

So I see two options here:

  • force SWIG to mess with datatypes in a way that int64_t will be casted to size_t, which may break on 32-bit
  • change the LGBM_DatasetGetFeatureNames to use platform-independent types for their arguments, which will break API compatibility
  • allow SWIG to correctly use the size_t type, so it will both work on 32bit and preserve API compat at the same time.

@AlbertoEAF
Copy link
Contributor

Sure shuttie, I'm not defending using int64_t, that's why I said:

I agree that is more correct to use a proper size_t pointer.

LGTM ;)

Copy link
Contributor

@imatiach-msft imatiach-msft left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Merging based on @AlbertoEAF's and @imatiach-msft's approvals. Thank you both guys!
@shuttie Thanks a lot got your contribution!

@StrikerRUS StrikerRUS merged commit a60d45c into microsoft:master Feb 5, 2021
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants