Re: [hackers] [st] [patch] use goto in xloadfonts

[Roberto E. Vargas Caballero]

Hi,

On Sat, Jun 20, 2015 at 01:32:45AM -0400, Michael Reed wrote:
  You just made the programmflow harder to grasp and removed the possibility
  to differentiate between the errors in the future. Also the patch adds 4
  SLoC without achieving anything.
 
 I agree it's harder to grasp, but only slightly, and I think the decrease in
 redundancy is worth it (although that's evidently subjective).

I personally don't think is harder. I think is a common pattern in
C to have die calls in the end of the function and gotos to them.
The main problem that I can see here is the label, whose name is
not significative enough: err, which error?, and if we have to add
more errors in the future, do we have to change this label and all
the gotos to this label?. I think a label like 'cant_open_font' is
better.  I usually like short names, but in the case of goto the
story is totally different, because you have a modification in the
control flow and it should be clear why it is done.

 I don't agree with the comment on error handling; it's not clear to me
 when/if this function will be refactored, but I don't know if gotos have
 complicated refactoring st in the past, so maybe you're right.
 

The problem here is different. St was broken in the past due to
style changes, and the history of the project is hard to read due
to this kind of patches, so several suckless developers agreed in
only accept patches which fix some error or add some feature. Style
changes will be accepted only if there is something else in the
patch. I like your patch (it makes st more similar to my style ;) ),
but due to this reason I will not apply it.


Regards,

[hackers] [st] [patch] use goto in xloadfonts

[Michael Reed]

From a52cadacaa937d74ebc66016641d6cde8116eba6 Mon Sep 17 00:00:00 2001
From: Michael Reed m.r...@mykolab.com
Date: Fri, 19 Jun 2015 16:58:04 -0400
Subject: [PATCH] use goto in xloadfonts()

---
 st.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/st.c b/st.c
index 666edec..ffd80df 100644
--- a/st.c
+++ b/st.c
@@ -3071,7 +3071,7 @@ xloadfonts(char *fontstr, double fontsize) {
 	}
 
 	if(!pattern)
-		die(st: can't open font %s\n, fontstr);
+		goto err;
 
 	if(fontsize  1) {
 		FcPatternDel(pattern, FC_PIXEL_SIZE);
@@ -3100,7 +3100,7 @@ xloadfonts(char *fontstr, double fontsize) {
 	FcDefaultSubstitute(pattern);
 
 	if(xloadfont(dc.font, pattern))
-		die(st: can't open font %s\n, fontstr);
+		goto err;
 
 	if(usedfontsize  0) {
 		FcPatternGetDouble(dc.font.match-pattern,
@@ -3117,19 +3117,23 @@ xloadfonts(char *fontstr, double fontsize) {
 	FcPatternDel(pattern, FC_SLANT);
 	FcPatternAddInteger(pattern, FC_SLANT, FC_SLANT_ITALIC);
 	if(xloadfont(dc.ifont, pattern))
-		die(st: can't open font %s\n, fontstr);
+		goto err;
 
 	FcPatternDel(pattern, FC_WEIGHT);
 	FcPatternAddInteger(pattern, FC_WEIGHT, FC_WEIGHT_BOLD);
 	if(xloadfont(dc.ibfont, pattern))
-		die(st: can't open font %s\n, fontstr);
+		goto err;
 
 	FcPatternDel(pattern, FC_SLANT);
 	FcPatternAddInteger(pattern, FC_SLANT, FC_SLANT_ROMAN);
 	if(xloadfont(dc.bfont, pattern))
-		die(st: can't open font %s\n, fontstr);
+		goto err;
 
 	FcPatternDestroy(pattern);
+	return;
+
+err:
+	die(st: can't open font %s\n, fontstr);
 }
 
 void
-- 
2.4.3