Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

Optimize C++ API#13496

Merged
marcoabreu merged 2 commits intoapache:masterfrom
zhaoyao73:master
Dec 12, 2018
Merged

Optimize C++ API#13496
marcoabreu merged 2 commits intoapache:masterfrom
zhaoyao73:master

Conversation

@zhaoyao73
Copy link
Contributor

@zhaoyao73 zhaoyao73 commented Nov 30, 2018

Pass parameter with reference instead of value.
Add const as well as it is not changed.

Description

Optimize some functions to pass reference instead of value. This could avoid unnecessary construction/destruction and copy.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • tiny changes
  • did a successful build
  • For user-facing API changes, API doc string has been updated.
    Didn't find the API doc in mxnet.io/doxygen
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Comments

I am not sure I could run a sanity test with it or how to run a unit test/sanity test against the change. But pass value -> pass reference, should be transparent to C++, although not like C's pass value->pass pointer.

@zhaoyao73
Optimize C++ API
523db4f 
Pass parameter with reference instead of value.
Add const as well as it is not changed.
@zhaoyao73 zhaoyao73 requested a review from nswamy as a code owner November 30, 2018 21:11
@vandanavk
Copy link
Contributor

vandanavk commented Nov 30, 2018

@mxnet-label-bot add [C++, pr-awaiting-review]

@leleamol for review

@marcoabreu marcoabreu added C++ Related to C++ pr-awaiting-review PR is waiting for code review labels Nov 30, 2018
@zhaoyao73
fix docs/architecture/overview.md
4bc8e5f 
Fix BinaryShapeFunction typedef
Add a right brace for SmoothL1Shape_
@zhaoyao73 zhaoyao73 requested a review from szha as a code owner December 4, 2018 19:00
sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Dec 4, 2018
View reviewed changes
Copy link

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

LGTM.
@leleamol

stu1130
stu1130 reviewed Dec 5, 2018
View reviewed changes
@zhaoyao73
Copy link
Contributor Author

zhaoyao73 commented Dec 6, 2018

any idea why centos-cpu still not done?
Is it the only thing need to be done?

@zhaoyao73
Copy link
Contributor Author

zhaoyao73 commented Dec 7, 2018

@marcoabreu May I ask why the centos-cpu still not done?

@marcoabreu
Copy link
Contributor

marcoabreu commented Dec 7, 2018

Seems like it hasn't been kicked off. Please rebase to start another build

larroy
larroy approved these changes Dec 7, 2018
View reviewed changes
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

LGTM

lebeg
lebeg approved these changes Dec 10, 2018
View reviewed changes
Copy link
Contributor

@lebeg lebeg left a comment

Choose a reason for hiding this comment

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

LGTM

@zhaoyao73
Copy link
Contributor Author

zhaoyao73 commented Dec 10, 2018

do I need to do anything?

@larroy
Copy link
Contributor

larroy commented Dec 10, 2018

@marcoabreu are we missing statuses from github? Check centos-cpu

@marcoabreu
Copy link
Contributor

marcoabreu commented Dec 10, 2018

@zhaoyao73 I have kicked off a build on your behalf. Sorry for the inconveniences.

@zhaoyao73
Copy link
Contributor Author

zhaoyao73 commented Dec 10, 2018

@marcoabreu just a dummy question, can I start a CI build?

@marcoabreu
Copy link
Contributor

marcoabreu commented Dec 10, 2018

Yeah, just make a new commit and it will automatically start a build

@larroy
Copy link
Contributor

larroy commented Dec 11, 2018

@mxnet-label-bot [pr-awaiting-merge]

@zhaoyao73
Copy link
Contributor Author

zhaoyao73 commented Dec 12, 2018

weird, look at this PR it is still pr-awaiting-review, although I see @larrroy put the pr-awaiting-merge?

@marcoabreu
Copy link
Contributor

marcoabreu commented Dec 12, 2018

You received two binding approvals, no requested changes and there are no outstanding discussions, thus the PR is ready to be merged.

@marcoabreu marcoabreu merged commit 002e0bb into apache:master Dec 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nswamy nswamy Awaiting requested review from nswamy nswamy is a code owner

@szha szha Awaiting requested review from szha szha is a code owner

5 more reviewers

@stu1130 stu1130 stu1130 left review comments

@larroy larroy larroy approved these changes

@lebeg lebeg lebeg approved these changes

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

@cjolivier01 cjolivier01 cjolivier01 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C++ Related to C++ pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@zhaoyao73 @vandanavk @marcoabreu @larroy @lebeg @sandeep-krishnamurthy @stu1130 @cjolivier01