Improve error handling to avoid memory leaks in C extension #278
Conversation
jacobperron
added
in review
| rcutils_ret_t fini_namespaces_ret; | ||
| cleanup: | ||
| fini_names_ret = rcutils_string_array_fini(&node_names); | ||
| fini_namespaces_ret = rcutils_string_array_fini(&node_namespaces); |
There was a problem hiding this comment.
@jacobperron nit: can't we declare and initialize on the same statement?
There was a problem hiding this comment.
Apparently, declarations directly after a label are not supported and results in compiler errors. This SO post suggest we could add an empty statement as a workaround. I don't feel strongly either way, but I'm leaning towards leaving it as-is because cleanup:; looks odd to me.
There was a problem hiding this comment.
I'm actually surprised you don't have warnings above where you declare local variables after the use of a goto. In the rmw implementations we declare all local variables just before the first use of goto, which is essentially what the compiler does even when you declare them throughout and I gather the use of goto might cause strange order of side-effects if you don't declare everything ahead of their use.
For example:
There was a problem hiding this comment.
Yeah, in retrospect this is odd.
I am able to produce an error if I try to both declare and initialize after a goto statement. But the compiler is fine if it's only a declaration. I've moved the declarations before the first goto statement so code reads better 67d3660
Signed-off-by: Jacob Perron <[email protected]>
…ment Signed-off-by: Jacob Perron <[email protected]>
jacobperron
force-pushed
the
jacob/c_ext_err_handling
branch
from
307b8e6
to
914ff23
Compare
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
|
Thanks for the reviews!
(I expect |
Edit: test failures are unrelated.