Fixed memory leak and some -Werror=unused-result warnings #2509
Conversation
OMGtechy
force-pushed
the
master
branch
2 times, most recently
from
d917cec to
ec60dc7
Compare
OMGtechy
force-pushed
the
master
branch
2 times, most recently
from
b12d8a6 to
a4b1d85
Compare
jsquyres
left a comment
jsquyres
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing -- December gets busy!
| Joshua Gerrard | ||
| enquiries@joshuagerrard.com | ||
| technopiano@gmail.com | ||
| joshuagerrard+ompi-commit@protonmail.com |
There was a problem hiding this comment.
@OMGtechy Are these the emails that you committed with? We typically only show the emails that show up in git log.
There was a problem hiding this comment.
One of them is - I've updated accordingly.
| ompi_process_info.nodename, | ||
| (int) ompi_process_info.pid) == -1) { | ||
| // non-fatal, we could still go on to give useful information here... | ||
| opal_output(0, "%s", "Could not write node and PID to prefix"); |
There was a problem hiding this comment.
Just curious -- why do the double-string form? You shouldn't need to use the copy through %s -- you could just put the 3rd argument directly as the 2nd arg.
There was a problem hiding this comment.
It's a habit to stop situations like this:
opal_output(0, user_input, ...)
Some compilers will also omit warnings if you don't do it this way, IIRC
There was a problem hiding this comment.
FWIW, I think compilers only warn on that for libc functions (e.g., printf). They don't for opal_output. But it doesn't matter that much...
| *error_code); | ||
| if (asprintf(&err_msg, unknown_error_code, | ||
| *error_code) == -1) { | ||
| opal_output(0, "%s", "Could not write to err_msg"); |
There was a problem hiding this comment.
Same comment as above -- %s doesn't seem to be needed...?
There was a problem hiding this comment.
See other comment :)
| asprintf(&err_msg, "Error code: %d (no associated error message)", | ||
| *error_code); | ||
| if (asprintf(&err_msg, unknown_error_code, | ||
| *error_code) == -1) { |
There was a problem hiding this comment.
Will a failed asprintf() leave NULL in err_msg? Or could it be some random value (that free(err_msg), below, will choke on)?
There was a problem hiding this comment.
Good spot! I also made the same fix for prefix
OMGtechy
force-pushed
the
master
branch
2 times, most recently
from
8911e66 to
c01bd92
Compare
No worries! Your review comments should be addressed now. |
Signed-off-by: Joshua Gerrard <joshuagerrard+ompi-commit@protonmail.com>
2 participants
Signed-off-by: Joshua Gerrard enquiries@joshuagerrard.com