Re: [PATCH 2/3] xdiff: refactor a function
From: Junio C Hamano <hidden>
Date: 2022-02-09 18:04:53
"Phillip Wood via GitGitGadget" [off-list ref] writes:
From: Phillip Wood <redacted> Rather than having to remember exactly what to free after an allocation failure use the standard "goto out" pattern. This will simplify the next commit that starts handling currently unhandled failures.
It sound like this is meant to be a no-op clean-up; let's see.
quoted hunk
Signed-off-by: Phillip Wood <redacted> --- xdiff/xmerge.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-)diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index fff0b594f9a..48c5e9e3f35 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c@@ -684,35 +684,30 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, xmparam_t const *xmp, mmbuffer_t *result) { - xdchange_t *xscr1, *xscr2; - xdfenv_t xe1, xe2; - int status; + xdchange_t *xscr1 = NULL, *xscr2 = NULL; + xdfenv_t xe1 = { 0 }, xe2 = { 0 }; + int status = -1; xpparam_t const *xpp = &xmp->xpp; result->ptr = NULL; result->size = 0; - if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) { - return -1; - } - if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) { - xdl_free_env(&xe1); - return -1; - }
OK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I didn't check if patience and histogram also do so correctly), so the original was not leaking xe1 or xe2.
+ if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) + goto out; + + if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) + goto out; +
And this does not change that.
if (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||
xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||
- xdl_build_script(&xe1, &xscr1) < 0) {
- xdl_free_env(&xe1);
- return -1;
- }
+ xdl_build_script(&xe1, &xscr1) < 0)
+ goto out;
+Here, as xdl_build_script() does free the script it failed to build, but not the xe it was given, the original is correct to free xe1. We jump from here to leave the freeing of xe1 to the clean-up part.
if (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||
xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||
- xdl_build_script(&xe2, &xscr2) < 0) {
- xdl_free_script(xscr1);
- xdl_free_env(&xe1);
- xdl_free_env(&xe2);
- return -1;
- }
+ xdl_build_script(&xe2, &xscr2) < 0)
+ goto out;Ditto for xe2 here.
quoted hunk
status = 0; if (!xscr1) { result->ptr = xdl_malloc(mf2->size);@@ -727,6 +722,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, &xe2, xscr2, xmp, result); } + out: xdl_free_script(xscr1); xdl_free_script(xscr2);
And after the post-context of this hunk, we do free_env() both xe1 and xe2, so we should be doing the same thing as before. Looking good.