Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
On Tue, Apr 19, 2022 at 08:06:19AM +0600, NRK wrote: > Hi, > > Any remaining problem with the patch? > > Assuming it just fell off the radar :) > > - NRK > Hi, Thanks for the reminder and the patch. It indeed fell off my radar, sorry about that. I pushed the patch now, -- Kind regards, Hiltjo
Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
Hi, Any remaining problem with the patch? Assuming it just fell off the radar :) - NRK
Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
On Mon, Mar 21, 2022 at 09:24:50AM +0100, Roberto E. Vargas Caballero wrote:
> I think is better to keep every ternary in a line by itself, because it makes
> easier to read them:
>
> fprintf(stderr, "erresc: failed to fetch %s color %d\n",
> is_osc4 ? "osc4" : "osc",
> is_osc4 ? num : index);
Done.
> I think we force here to put braces around if and else because the body of the
> if part is more of one line. Again, I think is better to use a line for every
> ternary and have something like:
>
>
> if (n < 0 || n >= sizeof(buf)) {
> fprintf(stderr, "error: %s while printing %s response\n",
> n < 0 ? "snprintf failed" : "truncation occurred",
> is_osc4 ? "osc4" : "osc");
> } else {
> ttywrite(buf, n, 1);
> }
>
> > + if ((j = par - 10) < 0 || j >= LEN(osc_table))
> > + break; /* shouldn't be possible */
> >
> > if (!strcmp(p, "?"))
> > - osc_color_response(defaultcs, 12);
> > - else if (xsetcolorname(defaultcs, p))
> > - fprintf(stderr, "erresc: invalid cursor color:
> > %s\n", p);
> > + osc_color_response(par, osc_table[j].idx, 0);
> > + else if (xsetcolorname(osc_table[j].idx, p))
> > + fprintf(stderr, "erresc: invalid %s color:
> > %s\n",
> > + osc_table[j].str, p);
> > else
> > tfulldirt();
> > return;
>
> Same apply here, I think our style forces to have braces in every if and else
> if
> because there is a body with more of one lines.
I agree with that, but I wasn't sure what the preferred style here is
since I've seen a lot of multi-line bodies have no braces, most notably
on the DWM source code. Though, looking around a bit, the ST source
seems to be consistent with it's braces.
Attached the amended patch.
- NRK
>From cab20470fd6c6f3832f10427b1970df5acf78245 Mon Sep 17 00:00:00 2001
From: NRK
Date: Fri, 7 Jan 2022 23:21:04 +0600
Subject: [PATCH] code-golfing: cleanup osc color related code
* adds missing function prototype
* move xgetcolor() prototype to win.h (that's where all the other x.c
func prototype seems to be declared at)
* check for snprintf error/truncation
* reduces code duplication for osc 10/11/12
* unify osc_color_response() and osc4_color_response() into a single function
the latter two was suggested by Quentin Rameau in his patch review on
the hackers list.
---
st.c | 90 +--
st.h | 2 --
win.h | 1 +
3 files changed, 33 insertions(+), 60 deletions(-)
diff --git a/st.c b/st.c
index c71fa06..7275da5 100644
--- a/st.c
+++ b/st.c
@@ -161,6 +161,7 @@ static void csidump(void);
static void csihandle(void);
static void csiparse(void);
static void csireset(void);
+static void osc_color_response(int, int, int);
static int eschandle(uchar);
static void strdump(void);
static void strhandle(void);
@@ -1843,39 +1844,28 @@ csireset(void)
}
void
-osc4_color_response(int num)
+osc_color_response(int num, int index, int is_osc4)
{
int n;
char buf[32];
unsigned char r, g, b;
- if (xgetcolor(num, &r, &g, &b)) {
- fprintf(stderr, "erresc: failed to fetch osc4 color %d\n", num);
+ if (xgetcolor(is_osc4 ? num : index, &r, &g, &b)) {
+ fprintf(stderr, "erresc: failed to fetch %s color %d\n",
+ is_osc4 ? "osc4" : "osc",
+ is_osc4 ? num : index);
return;
}
- n = snprintf(buf, sizeof buf, "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
- num, r, r, g, g, b, b);
-
- ttywrite(buf, n, 1);
-}
-
-void
-osc_color_response(int index, int num)
-{
- int n;
- char buf[32];
- unsigned char r, g, b;
-
- if (xgetcolor(index, &r, &g, &b)) {
- fprintf(stderr, "erresc: failed to fetch osc color %d\n", index);
- return;
+ n = snprintf(buf, sizeof buf, "\033]%s%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
+ is_osc4 ? "4;" : "", num, r, r, g, g, b, b);
+ if (n < 0 || n >= sizeof(buf)) {
+ fprintf(stderr, "error: %s while printing %s response\n",
+ n < 0 ? "snprintf failed" : "truncation occurred",
+ is_osc4 ? "osc4" : "osc");
+ } else {
+ ttywrite(buf, n, 1);
}
-
- n = snprintf(buf, sizeof buf, "\033]%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
- num, r, r, g, g, b, b);
-
- ttywrite(buf, n, 1);
}
void
@@ -1883,6 +1873,11 @@ strhandle(void)
{
char *p = NULL, *dec;
int j, narg, par;
+ const struct { int idx; char *str; } osc_table[] = {
+ { defaultfg, "foreground" },
+ { defaultbg, "background" },
+ { defaultcs, "cursor" }
+ };
term.esc &= ~(ESC_STR_END|ESC_STR);
strparse();
@@ -1917,43 +1912,22 @@ strhandle(void)
}
return;
case 10:
- if (narg < 2)
-break;
-
- p = strescseq.args[1];
-
Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
Hi,
A few small nitpicks about formating (fell free to ignore them
if you want ;)):
On Sun, Mar 20, 2022 at 06:25:40PM +0600, NRK wrote:
> @@ -1843,39 +1844,25 @@ csireset(void)
> }
>
> void
> -osc4_color_response(int num)
> +osc_color_response(int num, int index, int is_osc4)
> {
> int n;
> char buf[32];
> unsigned char r, g, b;
>
> - if (xgetcolor(num, &r, &g, &b)) {
> - fprintf(stderr, "erresc: failed to fetch osc4 color %d\n", num);
> + if (xgetcolor(is_osc4 ? num : index, &r, &g, &b)) {
> + fprintf(stderr, "erresc: failed to fetch %s color %d\n",
> + is_osc4 ? "osc4" : "osc", is_osc4 ? num : index);
I think is better to keep every ternary in a line by itself, because it makes
easier to read them:
fprintf(stderr, "erresc: failed to fetch %s color %d\n",
is_osc4 ? "osc4" : "osc",
is_osc4 ? num : index);
> + n = snprintf(buf, sizeof buf,
> "\033]%s%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
> + is_osc4 ? "4;" : "", num, r, r, g, g, b, b);
> + if (n < 0 || n >= sizeof(buf))
> + fprintf(stderr, "error: %s while printing %s response\n", n < 0
> ?
> + "snprintf failed" : "truncation occurred", is_osc4 ?
> "osc4" : "osc");
> + else
> + ttywrite(buf, n, 1);
> }
I think we force here to put braces around if and else because the body of the
if part is more of one line. Again, I think is better to use a line for every
ternary and have something like:
if (n < 0 || n >= sizeof(buf)) {
fprintf(stderr, "error: %s while printing %s response\n",
n < 0 ? "snprintf failed" : "truncation occurred",
is_osc4 ? "osc4" : "osc");
} else {
ttywrite(buf, n, 1);
}
> + if ((j = par - 10) < 0 || j >= LEN(osc_table))
> + break; /* shouldn't be possible */
>
> if (!strcmp(p, "?"))
> - osc_color_response(defaultcs, 12);
> - else if (xsetcolorname(defaultcs, p))
> - fprintf(stderr, "erresc: invalid cursor color:
> %s\n", p);
> + osc_color_response(par, osc_table[j].idx, 0);
> + else if (xsetcolorname(osc_table[j].idx, p))
> + fprintf(stderr, "erresc: invalid %s color:
> %s\n",
> + osc_table[j].str, p);
> else
> tfulldirt();
> return;
Same apply here, I think our style forces to have braces in every if and else if
because there is a body with more of one lines.
Regards,
Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
On Sun, Mar 20, 2022 at 12:49:22PM +0100, Hiltjo Posthuma wrote:
> > - n = snprintf(buf, sizeof buf,
> > "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
> > -num, r, r, g, g, b, b);
> > -
> > - ttywrite(buf, n, 1);
> > -}
>
> A nitpick, because I think the string always fits in the buffer `buf`, but:
> the
> return value can be negative or on truncation >= sizeof(buf). So the pattern
> of:
>
> n = snprintf(...);
> ttywrite(..., n, ...);
>
> Is not good.
Done, attached an amended patch. And on that topic, saw there was
another place using this pattern:
case 'n': /* DSR – Device Status Report (cursor position) */
if (csiescseq.arg[0] == 6) {
len = snprintf(buf, sizeof(buf), "\033[%i;%iR",
term.c.y+1, term.c.x+1);
ttywrite(buf, len, 0);
}
break;
Didn't touch it, since I think that's out of scope for this patch.
- NRK
>From 2ee30ead2ed4fab9625b14770c0857a8a07a7dc2 Mon Sep 17 00:00:00 2001
From: NRK
Date: Fri, 7 Jan 2022 23:21:04 +0600
Subject: [PATCH] code-golfing: cleanup osc color related code
* adds missing function prototype
* move xgetcolor() prototype to win.h (that's where all the other x.c
func prototype seems to be declared at)
* check for snprintf error/truncation
* reduces code duplication for osc 10/11/12
* unify osc_color_response() and osc4_color_response() into a single function
the latter two was suggested by Quentin Rameau in his patch review on
the hackers list.
---
st.c | 78 ++-
st.h | 2 --
win.h | 1 +
3 files changed, 25 insertions(+), 56 deletions(-)
diff --git a/st.c b/st.c
index c71fa06..12354ce 100644
--- a/st.c
+++ b/st.c
@@ -161,6 +161,7 @@ static void csidump(void);
static void csihandle(void);
static void csiparse(void);
static void csireset(void);
+static void osc_color_response(int, int, int);
static int eschandle(uchar);
static void strdump(void);
static void strhandle(void);
@@ -1843,39 +1844,25 @@ csireset(void)
}
void
-osc4_color_response(int num)
+osc_color_response(int num, int index, int is_osc4)
{
int n;
char buf[32];
unsigned char r, g, b;
- if (xgetcolor(num, &r, &g, &b)) {
- fprintf(stderr, "erresc: failed to fetch osc4 color %d\n", num);
+ if (xgetcolor(is_osc4 ? num : index, &r, &g, &b)) {
+ fprintf(stderr, "erresc: failed to fetch %s color %d\n",
+ is_osc4 ? "osc4" : "osc", is_osc4 ? num : index);
return;
}
- n = snprintf(buf, sizeof buf, "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
- num, r, r, g, g, b, b);
-
- ttywrite(buf, n, 1);
-}
-
-void
-osc_color_response(int index, int num)
-{
- int n;
- char buf[32];
- unsigned char r, g, b;
-
- if (xgetcolor(index, &r, &g, &b)) {
- fprintf(stderr, "erresc: failed to fetch osc color %d\n", index);
- return;
- }
-
- n = snprintf(buf, sizeof buf, "\033]%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
- num, r, r, g, g, b, b);
-
- ttywrite(buf, n, 1);
+ n = snprintf(buf, sizeof buf, "\033]%s%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
+ is_osc4 ? "4;" : "", num, r, r, g, g, b, b);
+ if (n < 0 || n >= sizeof(buf))
+ fprintf(stderr, "error: %s while printing %s response\n", n < 0 ?
+ "snprintf failed" : "truncation occurred", is_osc4 ? "osc4" : "osc");
+ else
+ ttywrite(buf, n, 1);
}
void
@@ -1883,6 +1870,11 @@ strhandle(void)
{
char *p = NULL, *dec;
int j, narg, par;
+ const struct { int idx; char *str; } osc_table[] = {
+ { defaultfg, "foreground" },
+ { defaultbg, "background" },
+ { defaultcs, "cursor" }
+ };
term.esc &= ~(ESC_STR_END|ESC_STR);
strparse();
@@ -1917,41 +1909,19 @@ strhandle(void)
}
return;
case 10:
- if (narg < 2)
-break;
-
- p = strescseq.args[1];
-
- if (!strcmp(p, "?"))
-osc_color_response(defaultfg, 10);
- else if (xsetcolorname(defaultfg, p))
-fprintf(stderr, "erresc: invalid foreground color: %s\n", p);
- else
-tfulldirt();
- return;
case 11:
- if (narg < 2)
-break;
-
- p = strescseq.args[1];
-
- if (!strcmp(p, "?"))
-osc_color_response(defaultbg, 11);
- else if (xsetcolorname(defaultbg, p))
-fprintf(stderr, "erresc: invalid background color: %s\n", p);
- else
-tfulldirt();
- return;
case 12:
if (narg < 2)
break;
-
p = strescseq.args[1];
+ if ((j = par - 10) < 0 || j >= LEN(osc_table))
+break; /* shouldn't be possible */
if (!strcmp(p, "?"))
-osc_color_response(defaultcs, 12);
- else if (xsetcolorname(defaultcs, p))
-fprintf(stderr, "erresc: invalid cursor color: %s\n", p);
+osc_color_response(par, osc_table[j].idx, 0);
+ else if (xsetcolorname(osc_table[j].idx, p))
+fprintf(stderr, "erresc: invalid %s color: %s\n",
+osc_table[j].str, p);
else
tfulldirt();
return;
@@ -1964,7 +1934,7 @@ strhandle(void)
j = (narg > 1) ? atoi(stres
Re: [hackers] [st][PATCH v2] code-golfing: cleanup osc color related code
On Sun, Mar 20, 2022 at 05:36:04PM +0600, NRK wrote:
> * adds missing function prototype
> * move xgetcolor() prototype to win.h (that's where all the other x.c
> func prototype seems to be declared at).
> * reduces code duplication for osc 10/11/12
> * unify osc_color_response() and osc4_color_response() into a single function.
>
> the latter two was suggested by Quentin Rameau in his patch review on
> the hackers list.
> ---
> st.c | 72 ---
> st.h | 2 --
> win.h | 1 +
> 3 files changed, 20 insertions(+), 55 deletions(-)
>
> diff --git a/st.c b/st.c
> index c71fa06..068c92c 100644
> --- a/st.c
> +++ b/st.c
> @@ -161,6 +161,7 @@ static void csidump(void);
> static void csihandle(void);
> static void csiparse(void);
> static void csireset(void);
> +static void osc_color_response(int, int, int);
> static int eschandle(uchar);
> static void strdump(void);
> static void strhandle(void);
> @@ -1843,38 +1844,20 @@ csireset(void)
> }
>
> void
> -osc4_color_response(int num)
> +osc_color_response(int num, int index, int is_osc4)
> {
> int n;
> char buf[32];
> unsigned char r, g, b;
>
> - if (xgetcolor(num, &r, &g, &b)) {
> - fprintf(stderr, "erresc: failed to fetch osc4 color %d\n", num);
> + if (xgetcolor(is_osc4 ? num : index, &r, &g, &b)) {
> + fprintf(stderr, "erresc: failed to fetch %s color %d\n",
> + is_osc4 ? "osc4" : "osc", is_osc4 ? num : index);
> return;
> }
>
> - n = snprintf(buf, sizeof buf,
> "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
> - num, r, r, g, g, b, b);
> -
> - ttywrite(buf, n, 1);
> -}
A nitpick, because I think the string always fits in the buffer `buf`, but: the
return value can be negative or on truncation >= sizeof(buf). So the pattern
of:
n = snprintf(...);
ttywrite(..., n, ...);
Is not good.
> -
> -void
> -osc_color_response(int index, int num)
> -{
> - int n;
> - char buf[32];
> - unsigned char r, g, b;
> -
> - if (xgetcolor(index, &r, &g, &b)) {
> - fprintf(stderr, "erresc: failed to fetch osc color %d\n",
> index);
> - return;
> - }
> -
> - n = snprintf(buf, sizeof buf,
> "\033]%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
> - num, r, r, g, g, b, b);
> -
> + n = snprintf(buf, sizeof buf,
> "\033]%s%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
> + is_osc4 ? "4;" : "", num, r, r, g, g, b, b);
> ttywrite(buf, n, 1);
> }
>
> @@ -1883,6 +1866,11 @@ strhandle(void)
> {
> char *p = NULL, *dec;
> int j, narg, par;
> + const struct { int idx; char *str; } osc_table[] = {
> + { defaultfg, "foreground" },
> + { defaultbg, "background" },
> + { defaultcs, "cursor" }
> + };
>
> term.esc &= ~(ESC_STR_END|ESC_STR);
> strparse();
> @@ -1917,41 +1905,19 @@ strhandle(void)
> }
> return;
> case 10:
> - if (narg < 2)
> - break;
> -
> - p = strescseq.args[1];
> -
> - if (!strcmp(p, "?"))
> - osc_color_response(defaultfg, 10);
> - else if (xsetcolorname(defaultfg, p))
> - fprintf(stderr, "erresc: invalid foreground
> color: %s\n", p);
> - else
> - tfulldirt();
> - return;
> case 11:
> - if (narg < 2)
> - break;
> -
> - p = strescseq.args[1];
> -
> - if (!strcmp(p, "?"))
> - osc_color_response(defaultbg, 11);
> - else if (xsetcolorname(defaultbg, p))
> - fprintf(stderr, "erresc: invalid background
> color: %s\n", p);
> - else
> - tfulldirt();
> - return;
> case 12:
> if (narg < 2)
> break;
> -
> p = strescseq.args[1];
> + if ((j = par - 10) < 0 || j >= LEN(osc_table))
> + break; /* shouldn't be possible */
>
> if (!strcmp(p, "?"))
> - osc_color_response(defaultcs, 12);
> - else if (xsetcolorname(defaultcs, p))
> - fprintf(stderr, "erresc: invalid cursor color:
> %s\n", p);
> + osc_color_response(par, osc_table[j].idx, 0);
> + else if (xsetcolorname(osc_table[j].idx, p))
> + fprintf(stderr, "erresc: invalid %s color:
> %s\n",
> + osc_table[j].str, p);
>
