# [PATCH] Simplify the code and avoid an attribution. 3 messages from 2007-11-22 to 2007-11-22. Participants: André Goddard Rosa, Mike Hommey, Junio C Hamano. Thread: https://gitlist.dev/t/10970 ## André Goddard Rosa, 2007-11-22 01:00 Subject: [PATCH] Simplify the code and avoid an attribution. Message-ID: URL: https://gitlist.dev/e/b8bf37780711211700m1aa32006t6302f134f9c3fb7e%40mail.gmail.com ``` Hi, all! Please cc: me as I'm not subscribed. I'm sending the patch inline only for review, probably it is mangled. Please use the attached patch if you agree with it. Sorry about sending it attached. >From cd0cc6995684e2801011910735146052e5b59ccc Mon Sep 17 00:00:00 2001 From: Andre Goddard Rosa Date: Tue, 27 Nov 2007 10:22:46 -0200 Subject: [PATCH] Simplify the code and avoid an attribution. Signed-off-by: Andre Goddard Rosa --- config.c | 19 ++++++++++--------- mailmap.c | 2 +- xdiff-interface.c | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/config.c b/config.c index 56e99fc..7c9fcdd 100644 --- a/config.c +++ b/config.c @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const char *filename) int ret; FILE *f = fopen(filename, "r"); - ret = -1; - if (f) { - config_file = f; - config_file_name = filename; - config_linenr = 1; - ret = git_parse_file(fn); - fclose(f); - config_file_name = NULL; - } + if (!f) + return -1; + + config_file = f; + config_file_name = filename; + config_linenr = 1; + ret = git_parse_file(fn); + fclose(f); + config_file_name = NULL; + return ret; } diff --git a/mailmap.c b/mailmap.c index 8714167..0c13ecd 100644 --- a/mailmap.c +++ b/mailmap.c @@ -7,7 +7,7 @@ int read_mailmap(struct path_list *map, const char *filename, char **repo_abbrev char buffer[1024]; FILE *f = fopen(filename, "r"); - if (f == NULL) + if (!f) return 1; while (fgets(buffer, sizeof(buffer), f) != NULL) { char *end_of_name, *left_bracket, *right_bracket; diff --git a/xdiff-interface.c b/xdiff-interface.c index be866d1..9dd1f3b 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -111,7 +111,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename) if (stat(filename, &st)) return error("Could not stat %s", filename); - if ((f = fopen(filename, "rb")) == NULL) + if (!(f = fopen(filename, "rb"))) return error("Could not open %s", filename); sz = xsize_t(st.st_size); ptr->ptr = xmalloc(sz); -- 1.5.3.6.861.gd794-dirty From cd0cc6995684e2801011910735146052e5b59ccc Mon Sep 17 00:00:00 2001 From: Andre Goddard Rosa Date: Tue, 27 Nov 2007 10:22:46 -0200 Subject: [PATCH] Simplify the code and avoid an attribution. Signed-off-by: Andre Goddard Rosa --- config.c | 19 ++++++++++--------- mailmap.c | 2 +- xdiff-interface.c | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/config.c b/config.c index 56e99fc..7c9fcdd 100644 --- a/config.c +++ b/config.c @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const char *filename) int ret; FILE *f = fopen(filename, "r"); - ret = -1; - if (f) { - config_file = f; - config_file_name = filename; - config_linenr = 1; - ret = git_parse_file(fn); - fclose(f); - config_file_name = NULL; - } + if (!f) + return -1; + + config_file = f; + config_file_name = filename; + config_linenr = 1; + ret = git_parse_file(fn); + fclose(f); + config_file_name = NULL; + return ret; } diff --git a/mailmap.c b/mailmap.c index 8714167..0c13ecd 100644 --- a/mailmap.c +++ b/mailmap.c @@ -7,7 +7,7 @@ int read_mailmap(struct path_list *map, const char *filename, char **repo_abbrev char buffer[1024]; FILE *f = fopen(filename, "r"); - if (f == NULL) + if (!f) return 1; while (fgets(buffer, sizeof(buffer), f) != NULL) { char *end_of_name, *left_bracket, *right_bracket; diff --git a/xdiff-interface.c b/xdiff-interface.c index be866d1..9dd1f3b 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -111,7 +111,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename) if (stat(filename, &st)) return error("Could not stat %s", filename); - if ((f = fopen(filename, "rb")) == NULL) + if (!(f = fopen(filename, "rb"))) return error("Could not open %s", filename); sz = xsize_t(st.st_size); ptr->ptr = xmalloc(sz); -- 1.5.3.6.861.gd794-dirty ``` ## Mike Hommey, 2007-11-22 20:01 Subject: Re: [PATCH] Simplify the code and avoid an attribution. Message-ID: <20071122200157.GC19675@glandium.org> URL: https://gitlist.dev/e/20071122200157.GC19675%40glandium.org In-Reply-To: ``` On Wed, Nov 21, 2007 at 11:00:02PM -0200, André Goddard Rosa wrote: > --- a/config.c > +++ b/config.c > @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const > char *filename) > int ret; > FILE *f = fopen(filename, "r"); > > - ret = -1; > - if (f) { > - config_file = f; > - config_file_name = filename; > - config_linenr = 1; > - ret = git_parse_file(fn); > - fclose(f); > - config_file_name = NULL; > - } > + if (!f) > + return -1; > + > + config_file = f; > + config_file_name = filename; > + config_linenr = 1; > + ret = git_parse_file(fn); > + fclose(f); > + config_file_name = NULL; > + > return ret; > } Actually, since it is more likely that the file has been opened, the original code is more optimal because it doesn't generate a jump in most cases. And if you're worried about the ret variable, don't worry, it's most likely stripped out by the compiler optimizations. > - if (f == NULL) > + if (!f) > - if ((f = fopen(filename, "rb")) == NULL) > + if (!(f = fopen(filename, "rb"))) It's a matter of taste Mike ``` ## Junio C Hamano, 2007-11-22 20:29 Subject: Re: [PATCH] Simplify the code and avoid an attribution. Message-ID: <7vk5oa3tyg.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk5oa3tyg.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071122200157.GC19675@glandium.org> ``` Mike Hommey writes: > On Wed, Nov 21, 2007 at 11:00:02PM -0200, André Goddard Rosa wrote: >> --- a/config.c >> +++ b/config.c >> @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const >> char *filename) >> int ret; >> FILE *f = fopen(filename, "r"); >> >> - ret = -1; >> - if (f) { >> - config_file = f; >> - config_file_name = filename; >> - config_linenr = 1; >> - ret = git_parse_file(fn); >> - fclose(f); >> - config_file_name = NULL; >> - } >> + if (!f) >> + return -1; >> + >> + config_file = f; >> + config_file_name = filename; >> + config_linenr = 1; >> + ret = git_parse_file(fn); >> + fclose(f); >> + config_file_name = NULL; >> + >> return ret; >> } > > Actually, since it is more likely that the file has been opened, the > original code is more optimal because it doesn't generate a jump in most > cases. And if you're worried about the ret variable, don't worry, it's > most likely stripped out by the compiler optimizations. I did not comment on this patch partly because I did not know what "attribution" meant. I think it is a good change from readability, not micro-optimization, point of view. If you structure your code this way, do this if (there is an error) return error code; do that do the rest it is much easier to read than do this set error code pessimistically if (it was successful) { do that update error code do the rest } return error code especially the more likely part that runs the real processing ("do that...do the rest") tends to grow over time, and even acquire other error checks. ```