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

Adjust Operator::Run to take reference instead of pointer #1168

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

klecki
Copy link
Contributor

@klecki klecki commented Aug 14, 2019

Signed-off-by: Krzysztof Lecki klecki@nvidia.com

Why we need this PR?

  • Refactoring to improve Operator API

What happened in this PR?

  • Explain solution of the problem, new feature added.
    The Operator::Run and RunImpl signatures are changed from accepting pointer to workspace to reference to workspace.
    This is more C++ way of things and matches the signature of Setup.
  • What was changed, added, removed?
    Run, RunImpl and similar signatures where changed from (Workspace *ws) to (Workspace &ws),
    access to ws changed from ws->x to ws.x. For some functions call was changed from fun(ws) to fun(&ws), as I intended to change only Run and related functions.
  • What is most important part that reviewers should focus on?
    Mostly mechanical changes. Check if there is no docs mismatch.
  • Was this PR tested? How?
    CI
  • Were docs and examples updated, if necessary?
    Yes, dummy plugin was also updated.

Would be nice to followup with adjusting GetArgument to take a ref.

JIRA TASK: [DALI-XXXX]

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Aug 14, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [857155]: BUILD STARTED

Copy link
Contributor

@JanuszL JanuszL left a comment

Choose a reason for hiding this comment

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

You can write something more than just yes in What was changed, added, removed?.
Other than that LGTM.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [857155]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [857155]: BUILD PASSED

@klecki
Copy link
Contributor Author

klecki commented Aug 15, 2019

You can write something more than just yes in What was changed, added, removed?.
Other than that LGTM.

Done. :(
I was also considering a table:

Was before Is now Where used
Workspace *ws Workspace &ws Function argument
Workspace* ws Workspace &ws Function argument
Workspace * ws Workspace &ws Function argument
ws->x(); ws.x(); Member access to workspce
fun(ws) fun(&ws) Run[Impl] calling a function accepting pointer to ws

@klecki
Copy link
Contributor Author

klecki commented Aug 15, 2019

Regarding the Operator::RunImpl, as some users with custom ops may be using it, I can maybe provide the old overload there with [[deprecated]] attribute, leave it for one release and remove in the next one. What do you think? We don't promise that the C++ API won't change.

@JanuszL
Copy link
Contributor

JanuszL commented Aug 15, 2019

Regarding the Operator::RunImpl, as some users with custom ops may be using it, I can maybe provide the old overload there with [[deprecated]] attribute, leave it for one release and remove in the next one. What do you think? We don't promise that the C++ API won't change.

Sounds good. I guess we cannot put any compilation time warning for the users so they know they need to change something sooner than we remove it in the next release.

@klecki
Copy link
Contributor Author

klecki commented Aug 19, 2019

Regarding the Operator::RunImpl, as some users with custom ops may be using it, I can maybe provide the old overload there with [[deprecated]] attribute, leave it for one release and remove in the next one. What do you think? We don't promise that the C++ API won't change.

Sounds good. I guess we cannot put any compilation time warning for the users so they know they need to change something sooner than we remove it in the next release.

I'm still thinking about it, and it would require us to call old signature as a fallback in the new one, which makes the RunImpl not pure virtual. I don't think it is worth it.

@klecki klecki merged commit a9989e0 into NVIDIA:master Aug 19, 2019
@klecki klecki deleted the ws-ref branch August 19, 2019 11:52
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.

4 participants