refactor of git_setup_gettext method
PATCH 1/1 refactor of git_setup_gettext method
From: Dima Kov <[email protected]>
Use one "free" call at the end of the function,
instead of being dependent on the execution flow.
---
refactor of git_setup_gettext method
Use one "free" call at the end of the function, instead of being dependent on the execution flow.
Hi. My first PR fot Git repository. I went over the code and saw that maybe this part can be more clearer. Thanks.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-964%2Fdimakov%2Ffree_opt-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-964/dimakov/free_opt-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/964
gettext.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
gettext.c +6 -9
@@ -109,17 +109,14 @@ void git_setup_gettext(void)if (!podir) podir = p =system_path (GIT_LOCALE_PATH ); -if (!is_directory (podir)) { -free (p); -return ; +if (is_directory (podir)) { +bindtextdomain ("git" , podir); +setlocale (LC_MESSAGES ,"" ); +setlocale (LC_TIME ,"" ); +init_gettext_charset ("git" ); +textdomain ("git" ); } -bindtextdomain ("git" , podir); -setlocale (LC_MESSAGES ,"" ); -setlocale (LC_TIME ,"" ); -init_gettext_charset ("git" ); -textdomain ("git" ); -free (p); }
base-commit: de88ac70f3a801262eb3aa087e5d9a712be0a54a
Signature · 1 line
gitgitgadget
Quoted 7 lines · Dima Kov via GitGitGadget
Use one "free" call at the end of the function,
instead of being dependent on the execution flow.Signed-off-by Dima Kov <[email protected]>---
Hi. My first PR fot Git repository. I went over the code and saw that maybe this part can be more clearer. Thanks.
Thanks for taking the time to submit a patch to the project. Your change looks "obviously correct", however...
Quoted 10 lines
gettext.c +5
-3@@ -109,17 +109,14 @@ void git_setup_gettext(void) -if (!is_directory (podir)) { -free (p); -return ; +if (is_directory (podir)) { +bindtextdomain ("git" , podir); +setlocale (LC_MESSAGES ,"" ); +setlocale (LC_TIME ,"" ); +init_gettext_charset ("git" );
gettext.c void git_setup_gettext(void) · line 117
Quoted 10 lines
gettext.c +1
-6+textdomain ("git" ); } -bindtextdomain ("git" , podir); -setlocale (LC_MESSAGES ,"" ); -setlocale (LC_TIME ,"" ); -init_gettext_charset ("git" ); -textdomain ("git" ); -free (p);
... "clearness" is subjective, and it turns out that this project generally prefers the code the way it was before this patch, and you will find a lot of similar code throughout the project. In particular, we like to get the simple cases and the error cases out of the way early so that we don't have to think about them again for the remainder of the function. Doing it this way eases cognitive load. (A minor side benefit is that it also saves one or more levels of indentation.)
An alternative which is also crops up somewhat often in this project is to use goto, like this:
if (!is_directory(podir))
goto done;
bindtextdomain(...);
...
done:
free(p);However, goto is most often used when there is a lot of cleanup to do or the cleanup is intricate. This specific case doesn't qualify as either, so the code is probably fine as-is.
Quoted 2 lines · Dima Kov via GitGitGadget
From: Dima Kov <[email protected]>
Subject: Re: [PATCH] refactor ofgit_setup_gettextmethod
We do not write in C++, so "method" -> "function".
Quoted 5 lines
Use one "free" call at the end of the function,
instead of being dependent on the execution flow.Signed-off-by Dima Kov <[email protected]>---
I think an early return is more readable, but if this were a new code and used the style used in this patch, it would also have been acceptable. IOW, this is probably a borderline "Meh" change, belonging to "what's already commited is good enough and it is not worth the brain cycles to swap it around" category.
Quoted 33 lines
gettext.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)gettext.c +6
-9@@ -109,17 +109,14 @@ void git_setup_gettext(void)if (!podir) podir = p =system_path (GIT_LOCALE_PATH ); -if (!is_directory (podir)) { -free (p); -return ; +if (is_directory (podir)) { +bindtextdomain ("git" , podir); +setlocale (LC_MESSAGES ,"" ); +setlocale (LC_TIME ,"" ); +init_gettext_charset ("git" ); +textdomain ("git" ); } -bindtextdomain ("git" , podir); -setlocale (LC_MESSAGES ,"" ); -setlocale (LC_TIME ,"" ); -init_gettext_charset ("git" ); -textdomain ("git" ); -free (p); }base-commit:
de88ac70f3a801262eb3aa087e5d9a712be0a54a
Hi Dima, welcome to Git mailing list!
Quoted 15 lines
gettext.c +4
-3@@ -109,17 +109,14 @@ void git_setup_gettext(void)if (!podir) podir = p =system_path (GIT_LOCALE_PATH ); -if (!is_directory (podir)) { -free (p); -return ; +if (is_directory (podir)) { +bindtextdomain ("git" , podir); +setlocale (LC_MESSAGES ,"" ); +setlocale (LC_TIME ,"" );
gettext.c void git_setup_gettext(void) · line 122
Quoted 10 lines
gettext.c +2
-6+init_gettext_charset ("git" ); +textdomain ("git" ); } -bindtextdomain ("git" , podir); -setlocale (LC_MESSAGES ,"" ); -setlocale (LC_TIME ,"" ); -init_gettext_charset ("git" ); -textdomain ("git" ); -
It seems like you move gettext initialization into if body, so the patch title should be better say "move gettext initialization to if body".
gettext.c void git_setup_gettext(void) · line 122
Quoted 3 lines
gettext.c +0
-0free (p); }
Thanks for review.
Signature · 1 line
An old man doll... just what I always wanted! - Clara