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

re-define flux_subprocess_destroy prototype #1658

Merged
merged 2 commits into from Sep 13, 2018

Conversation

Projects
None yet
3 participants
@chu11
Copy link
Contributor

chu11 commented Sep 12, 2018

Per a discussion in #1564, re-define flux_subprocess_destroy() as a convenience that calls flux_subprocess_unref(). Also, includes a dumb typo fix I found.

chu11 added some commits Sep 12, 2018

common/subprocess: re-define flux_subprocess_destroy
Re-define flux_subprocess_destroy() to point to flux_subprocess_unref().

Any functions that required flux_subprocess_destroy to take a void *,
create a local wrapper function instead.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2018

Coverage Status

Coverage decreased (-0.04%) to 79.382% when pulling 680473a on chu11:issue1331-cleanup2 into b7b5e18 on flux-framework:master.

@@ -310,7 +310,7 @@ flux_future_t *flux_subprocess_kill (flux_subprocess_t *p, int signo);
*/
void flux_subprocess_ref (flux_subprocess_t *p);
void flux_subprocess_unref (flux_subprocess_t *p);
void flux_subprocess_destroy (void *arg);
#define flux_subprocess_destroy(x) flux_subprocess_unref(x)

This comment has been minimized.

@grondo

grondo Sep 13, 2018

Contributor

Seems like you probably don't need to export the #define for flux_subprocess_destroy(). The API should just specify to use unref, unless I'm missing some reason the API requires a destroy?

This comment has been minimized.

@chu11

chu11 Sep 13, 2018

Author Contributor

It was primarily for convenience, b/c destroy is the common function name we use for this.

Could go either way. Do you think it would be cleaner to just remove it outright?

This comment has been minimized.

@grondo

grondo Sep 13, 2018

Contributor

Actually, your approach is fine. For some reason I was thinking we had other examples in flux-core that used the ref/unref incref/decref interface, but I must have been thinking of jansson.

So your approach was correct, sorry.

@grondo grondo merged commit 5435246 into flux-framework:master Sep 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.04%) to 79.382%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.