Re: [hackers] [st] [patch] use goto in xloadfonts
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,
Re: [hackers] [st] [patch] use goto in xloadfonts
On Mon, 22 Jun 2015 09:26:17 +0200 "Roberto E. Vargas Caballero" wrote: Hey Roberto, just chiming in into this interesting discussion, not in the interest of bloating it even more, though. > 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 couldn't agree more. I followed the LibreSSL-refactoring very closely and they did the same. Gotos + cleanup at the end instead of local bailouts; the names need to be descriptive though. Could you give me some information if a cleanup is necessary when the font fails to load? Is there some possibility open with the gotos now to have some kind of cleanup? In general, I prefer this kind of error-handling way over exceptions. :) Cheers FRIGN -- FRIGN
Re: [hackers] [st] [patch] use goto in xloadfonts
FRIGN wrote: > "Roberto E. Vargas Caballero" wrote: > > 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. > > Could you give me some information if a cleanup is necessary when the > font fails to load? Is there some possibility open with the gotos now to > have some kind of cleanup? > In general, I prefer this kind of error-handling way over exceptions. :) Heyho, although this is already decided I have some further points to consider in further decisions: I also use this scheme regularly, but I don't think it is useful here since: - there is no cleanup done in any case - we don't have to set different return values - It's just a plain call to die() here. The complexity with the goto comes from the fact, that the reader has to „jump“ to the function end instead of just seeing the die() call directly. Of course if the rest of st would use the goto scheme, consistency would be a good argument (more than enough to overweigh the previous ones) to use it here too even if it's not needed. The behaviour that st dies instantly if a font could not be loaded is not so good, so we could output a warning and/or use a fallback font or use libsl in st (I think that has already been discussed). --Markus
[hackers] [sbase] Remove debug-code in printf(1) || FRIGN
commit d0269af73e099e4f7dc80e40ce00ed554c4fa35a
Author: FRIGN
AuthorDate: Sun Jun 21 15:57:32 2015 +0200
Commit: sin
CommitDate: Mon Jun 22 20:03:55 2015 +0100
Remove debug-code in printf(1)
diff --git a/printf.c b/printf.c
index d5d852d..72cbe46 100644
--- a/printf.c
+++ b/printf.c
@@ -50,7 +50,7 @@ main(int argc, char *argv[])
/* flag */
for (flag = '\0', i++; strchr("#-+ 0", format[i]); i++) {
- if (!flag) flag = format[i];
+ flag = format[i];
}
/* field width */
[hackers] [sbase] Fix parameter-usage in printf(1) || FRIGN
commit d848bcac4b1160f9e05a9acf5d00d0418285eec3
Author: FRIGN
AuthorDate: Sun Jun 21 15:52:21 2015 +0200
Commit: sin
CommitDate: Mon Jun 22 20:03:54 2015 +0100
Fix parameter-usage in printf(1)
1) Don't default to a space for numeric conversions. Instead,
set flag to 0 and work with it on a case-basis.
This fixes the wrong output of "printf %d 20" which had a
space prepended to it (" 20").
2) Add precision for doiuxX, which is zero-padding.
This fixes the wrong output of "printf %.5d 20" to properly
print "00020".
Thanks to emg for reporting these!
diff --git a/printf.c b/printf.c
index 1335906..d5d852d 100644
--- a/printf.c
+++ b/printf.c
@@ -49,8 +49,8 @@ main(int argc, char *argv[])
}
/* flag */
- for (flag = ' ', i++; strchr("#-+ 0", format[i]); i++) {
- flag = format[i];
+ for (flag = '\0', i++; strchr("#-+ 0", format[i]); i++) {
+ if (!flag) flag = format[i];
}
/* field width */
@@ -135,18 +135,21 @@ main(int argc, char *argv[])
rarg = ereallocarray(NULL, utflen(arg) + 1,
sizeof(*rarg));
utftorunestr(arg, rarg);
num = rarg[0];
- } else
+ } else {
num = (strlen(arg) > 0) ? estrtonum(arg,
LLONG_MIN, LLONG_MAX) : 0;
- fmt = estrdup("%#*ll#");
- fmt[1] = flag;
- fmt[5] = format[i];
- printf(fmt, width, num);
+ }
+ fmt = estrdup(flag ? "%#*.*ll#" : "%*.*ll#");
+ if (flag)
+ fmt[1] = flag;
+ fmt[flag ? 7 : 6] = format[i];
+ printf(fmt, width, precision, num);
free(fmt);
break;
case 'a': case 'A': case 'e': case 'E': case 'f': case 'F':
case 'g': case 'G':
- fmt = estrdup("%#*.*#");
- fmt[1] = flag;
- fmt[5] = format[i];
+ fmt = estrdup(flag ? "%#*.*#" : "%*.*#");
+ if (flag)
+ fmt[1] = flag;
+ fmt[flag ? 5 : 4] = format[i];
dou = (strlen(arg) > 0) ? estrtod(arg) : 0;
printf(fmt, width, precision, dou);
free(fmt);
