Fixed -Werror=unused-result warnings in comm_cid.c by adding error checking #2457
Conversation
|
@hjelmn please review |
jsquyres
left a comment
jsquyres
left a comment
There was a problem hiding this comment.
@OMGtechy Thank you for the submission!
I have a few minor comments on the code. Also, if you don't want to put your email address on your signed off line, you can probably just use (@OMGtechy) (the @ should indicate that it's a Github ID).
ompi/communicator/comm_cid.c
Outdated
| opal_pmix_pdata_t pdat; | ||
| opal_buffer_t sbuf; | ||
| int rc; | ||
| int bytesWritten; |
There was a problem hiding this comment.
Minor nit: we don't typically use camelCase in Open MPI; we typically use multiple_words. This is pretty trivial -- we would not reject a patch just for this reason, though -- I mention it just for "style fit" reasons.
ompi/communicator/comm_cid.c
Outdated
| fprintf (stderr, "port string: %s\n", cid_context->port_string); | ||
| fprintf (stderr, "pmix tag: %s\n", cid_context->pmix_tag); | ||
| fprintf (stderr, "iter: %d\n", cid_context->iter); | ||
| return OMPI_ERR_FATAL; |
There was a problem hiding this comment.
You can probably return OMPI_ERR_OUT_OF_RESOURCE here; it should be treated as a fatal error by the caller.
ompi/communicator/comm_cid.c
Outdated
| cid_context->iter); | ||
|
|
||
| if (bytesWritten == -1) { | ||
| fprintf (stderr, "writing info.key failed\n"); |
There was a problem hiding this comment.
We don't typically fprintf() from deep within the library. Instead, we typically use opal_output() if something needs to unconditionally be written to stderr (because it will prefix the output with a label indicating the process it came from). For errors like this, however, we typically will use opal_output_verbose(), which lets the error only be displayed conditionally (i.e., if the user has selected a high enough verbose value at run time), or just let the caller decide whether to display the error or not.
There was a problem hiding this comment.
Thank you! I used fprintf because other bits of code around here used it (fprintf (stderr, "pack failed. rc %d\n", rc); for example). Would it be worth me fixing that in another pull request, or is there a specific reason in that case?
There was a problem hiding this comment.
We used fprintf elsewhere in here? Oh, how embarrassing... 😳
Yes, if you could have a 2nd commit on this PR that fixes all the other fprintfs, that would be great. Thanks!
|
bot:ibm:retest |
OMGtechy
left a comment
OMGtechy
left a comment
There was a problem hiding this comment.
I believe I've fixed all review comments now :)
|
Please squish. |
|
Sweet; thanks! One last request: can you squash down to just two commits?
That way, we don't have a commit followed by multiple followup commits to fix the elements identified in the review. |
…ecking Signed-off-by: Joshua Gerrard <enquiries@joshuagerrard.com>
|
Works for me; thanks. Will merge when CI finishes. |
5 participants
This is my first commit (and therefore pull request), so please let me know if I've not followed any of your conventions.
This pull request does the following: