-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
@mxnet-label-bot add [pr-work-in-progress] |
99cb9d8
to
957cda4
Compare
@mxnet-label-bot add [pr-awaiting-review] |
@apeforest @ChaiBapchya pr is ready to review |
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.
LGTM!
957cda4
to
d38aeb7
Compare
@@ -43,8 +43,6 @@ extern "C" { | |||
|
|||
/*! \brief manually define unsigned int */ | |||
typedef uint32_t mx_uint; |
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.
Why do we still need this?
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.
yes we do other language bindings use this
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.
Which language bindings use this?
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.
java, C++, R, Scala
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.
Can you point me to the line in this bindings? It looks weird if this typedef is not used in this file while still keeping it.
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.
Few examples in code:
@@ -43,8 +43,6 @@ extern "C" { | |||
|
|||
/*! \brief manually define unsigned int */ | |||
typedef uint32_t mx_uint; | |||
/*! \brief manually define 64-bit int */ | |||
typedef int64_t mx_int64; | |||
/*! \brief manually define float */ | |||
typedef float mx_float; |
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.
Why do we still need this?
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.
yes we do other language bindings use this
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.
Which language bindings use this?
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.
java, C++, R, Scala
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 did a search in MXNet codebase and could not find any. Could you point me to the line?
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.
Thanks for the refactoring. This will increase readability and make C API users less dependency as well. I guess you also need to update the python interface that invokes the C API?
@apeforest This PR only contains code cleanup from C API end. Python cleanup needs to be done in a separate PR. typedefs should remain as is because other language bindings use them. |
Why can't it be done in one PR? Isn't that what the purpose of this PR is? |
d38aeb7
to
9bfb654
Compare
These are independent changes. I think these can be done in separate PRs. This is also keep C changes and individual language binding related changes separate, keeping each PR small |
9bfb654
to
6763a08
Compare
b0f7545
to
2ce1c21
Compare
* change mx_uint->int32_t * changing mx_int64 -> int64_t * change mx_float -> float * maintining typedefs for other language bindings * Re-Trigger build
* change mx_uint->int32_t * changing mx_int64 -> int64_t * change mx_float -> float * maintining typedefs for other language bindings * Re-Trigger build
Description
changes
removes
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.