date:20220626

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-26 Thread Quentin Rameau

Hi Tom,

> Let the Makefile be a bit more verbose and remove unnecessary extensions
> and flags in config.mk.
> ---
  
>  cscope: ${SRC} config.h
>   @echo cScope
> - @cscope -R -b || echo cScope not installed
> + cscope -R -b || echo cScope not installed

I think that you can also drop the || altogether,
make will just tell you whatever problem it encountered
while trying to invoque cscope.

> - @mkdir -p ${DESTDIR}${MANPREFIX}/man1
> - @cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
> - @chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
> + mkdir -p ${DESTDIR}${MANPREFIX}/man1
> + cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1

I'd cp -f here too.

>  # includes and libs
> -INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
> -LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> +INCS = -I/usr/include/freetype2 -I${X11INC}
> +LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11

The -L option should go into LDFLAGS

>  # flags
>  CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
> -CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
> -LDFLAGS += -g ${LIBS}
> -#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
> -#LDFLAGS += ${LIBS}
> +CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes -Wold-style-definition 
> -Os ${INCS} ${CPPFLAGS}

The -std option isn't necessary with the default CC.
I'd remove any warning from production build,
warnings are for development,
then developpers can set whatever warnings they like.

>  # compiler and linker
> -CC ?= cc
> +CC = cc

This should be removed altogether if you want a standard Makefile,
then CC will just be the expected c99.

Re: [hackers] [sent] [PATCH 1/3] sent.c: Drop unnecessary NULL checks

2022-06-26 Thread Laslo Hunhold

On Sun, 26 Jun 2022 21:53:49 +0300
Greg Minshall  wrote:

Dear Greg,

> for what it's worth, i'd probably code with the checks, so as to avoid
> future code editors (including myself) doing a double-take, thinking,
> "hmm, did the author consider that case?".  (though, of course, you
> did that same -- if opposite -- double-take when you saw that code.)

come on, it is general knowledge that free() accepts NULL arguments.
The extra checks just add more cruft.

With best regards

Laslo

Re: [hackers] [sent] [PATCH 1/3] sent.c: Drop unnecessary NULL checks

2022-06-26 Thread Greg Minshall

Tom,

> Since no operation is performed when free(3) is used on NULL, remove
> the checks.

for what it's worth, i'd probably code with the checks, so as to avoid
future code editors (including myself) doing a double-take, thinking,
"hmm, did the author consider that case?".  (though, of course, you did
that same -- if opposite -- double-take when you saw that code.)

cheers, Greg

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-26 Thread Hiltjo Posthuma

On Sun, Jun 26, 2022 at 11:07:52AM +, Tom Schwindl wrote:
> Let the Makefile be a bit more verbose and remove unnecessary extensions
> and flags in config.mk.
> ---
>  Makefile  | 37 +++--
>  config.mk | 18 --
>  2 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 56e636734216..d73909283970 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,5 +1,6 @@
>  # sent - plain text presentation tool
>  # See LICENSE file for copyright and license details.
> +.POSIX:
>  
>  include config.mk
>  
> @@ -18,43 +19,43 @@ config.h:
>   cp config.def.h config.h
>  
>  .c.o:
> - @echo CC $<
> - @${CC} -c ${CFLAGS} $<
> + @echo ${CC} $<
> + ${CC} -c ${CFLAGS} $<
>  
>  ${OBJ}: config.h config.mk
>  
>  sent: ${OBJ}
> - @echo CC -o $@
> - @${CC} -o $@ ${OBJ} ${LDFLAGS}
> + @echo ${CC} -o $@
> + ${CC} -o $@ ${OBJ} ${LDFLAGS}
>  
>  cscope: ${SRC} config.h
>   @echo cScope
> - @cscope -R -b || echo cScope not installed
> + cscope -R -b || echo cScope not installed
>  
>  clean:
>   @echo cleaning
> - @rm -f sent ${OBJ} sent-${VERSION}.tar.gz
> + rm -f sent ${OBJ} sent-${VERSION}.tar.gz
>  
>  dist: clean
>   @echo creating dist tarball
> - @mkdir -p sent-${VERSION}
> - @cp -R LICENSE Makefile config.mk config.def.h ${SRC} sent-${VERSION}
> - @tar -cf sent-${VERSION}.tar sent-${VERSION}
> - @gzip sent-${VERSION}.tar
> - @rm -rf sent-${VERSION}
> + mkdir -p sent-${VERSION}
> + cp -R LICENSE Makefile config.mk config.def.h ${SRC} sent-${VERSION}
> + tar -cf sent-${VERSION}.tar sent-${VERSION}
> + gzip sent-${VERSION}.tar
> + rm -rf sent-${VERSION}
>  
>  install: all
>   @echo installing executable file to ${DESTDIR}${PREFIX}/bin
> - @mkdir -p ${DESTDIR}${PREFIX}/bin
> - @cp -f sent ${DESTDIR}${PREFIX}/bin
> - @chmod 755 ${DESTDIR}${PREFIX}/bin/sent
> + mkdir -p ${DESTDIR}${PREFIX}/bin
> + cp -f sent ${DESTDIR}${PREFIX}/bin
> + chmod 755 ${DESTDIR}${PREFIX}/bin/sent
>   @echo installing manual page to ${DESTDIR}${MANPREFIX}/man1
> - @mkdir -p ${DESTDIR}${MANPREFIX}/man1
> - @cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
> - @chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
> + mkdir -p ${DESTDIR}${MANPREFIX}/man1
> + cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
> + chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
>  
>  uninstall:
>   @echo removing executable file from ${DESTDIR}${PREFIX}/bin
> - @rm -f ${DESTDIR}${PREFIX}/bin/sent
> + rm -f ${DESTDIR}${PREFIX}/bin/sent
>  
>  .PHONY: all options clean dist install uninstall cscope
> diff --git a/config.mk b/config.mk
> index d61c55437f46..15fdcfa10dc7 100644
> --- a/config.mk
> +++ b/config.mk
> @@ -11,20 +11,18 @@ X11INC = /usr/X11R6/include
>  X11LIB = /usr/X11R6/lib
>  
>  # includes and libs
> -INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
> -LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> +INCS = -I/usr/include/freetype2 -I${X11INC}
> +LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
>  # OpenBSD (uncomment)
> -#INCS = -I. -I${X11INC} -I${X11INC}/freetype2
> +#INCS = -I${X11INC} -I${X11INC}/freetype2
>  # FreeBSD (uncomment)
> -#INCS = -I. -I/usr/local/include -I/usr/local/include/freetype2 -I${X11INC}
> -#LIBS = -L/usr/local/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> +#INCS = -I/usr/local/include/freetype2 -I${X11INC}
> +#LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
>  
>  # flags
>  CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
> -CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
> -LDFLAGS += -g ${LIBS}
> -#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
> -#LDFLAGS += ${LIBS}
> +CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes -Wold-style-definition 
> -Os ${INCS} ${CPPFLAGS}
> +LDFLAGS = ${LIBS}
>  
>  # compiler and linker
> -CC ?= cc
> +CC = cc
> -- 
> 2.36.1
> 
> 

I would remove the echo also for the verbose changes.

I see the CFLAGS is also changed. You should document it in the commit message.
I'm not sure I agree with adding -Wall or the many -Woptions though.

It is up to the current sent maintainer to decide.

-- 
Kind regards,
Hiltjo

[hackers] [sent] [PATCH 2/3] treewide: Improve compliance with our coding style

2022-06-26 Thread Tom Schwindl

- Add `void` as parameter for functions which do not take any Arguments
- Use a block if the body of a loop consists of an `if` statement
- A few minor changes such as moving a curley bracket
---
 drw.c  |  3 ++-
 sent.c | 69 +-
 util.c |  3 ++-
 3 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/drw.c b/drw.c
index c1582e746cc5..84f87b972384 100644
--- a/drw.c
+++ b/drw.c
@@ -19,9 +19,10 @@ static const long utfmax[UTF_SIZ + 1] = {0x10, 0x7F, 
0x7FF, 0x, 0x10
 static long
 utf8decodebyte(const char c, size_t *i)
 {
-   for (*i = 0; *i < (UTF_SIZ + 1); ++(*i))
+   for (*i = 0; *i < (UTF_SIZ + 1); ++(*i)) {
if (((unsigned char)c & utfmask[*i]) == utfbyte[*i])
return (unsigned char)c & ~utfmask[*i];
+   }
return 0;
 }
 
diff --git a/sent.c b/sent.c
index 9534fcaf97b9..a27b704cd5ce 100644
--- a/sent.c
+++ b/sent.c
@@ -99,12 +99,12 @@ static void load(FILE *fp);
 static void advance(const Arg *arg);
 static void quit(const Arg *arg);
 static void resize(int width, int height);
-static void run();
-static void usage();
-static void xdraw();
-static void xhints();
-static void xinit();
-static void xloadfonts();
+static void run(void);
+static void usage(void);
+static void xdraw(void);
+static void xhints(void);
+static void xinit(void);
+static void xloadfonts(void);
 
 static void bpress(XEvent *);
 static void cmessage(XEvent *);
@@ -116,12 +116,12 @@ static void configure(XEvent *);
 #include "config.h"
 
 /* Globals */
-static const char *fname = NULL;
-static Slide *slides = NULL;
-static int idx = 0;
-static int slidecount = 0;
+static const char *fname;
+static Slide *slides;
+static int idx;
+static int slidecount;
 static XWindow xw;
-static Drw *d = NULL;
+static Drw *d;
 static Clr *sc;
 static Fnt *fonts[NUMFONTSCALES];
 static int running = 1;
@@ -326,9 +326,10 @@ getfontsize(Slide *s, unsigned int *width, unsigned int 
*height)
float lfac = linespacing * (s->linecount - 1) + 1;
 
/* fit height */
-   for (j = NUMFONTSCALES - 1; j >= 0; j--)
+   for (j = NUMFONTSCALES - 1; j >= 0; j--) {
if (fonts[j]->h * lfac <= xw.uh)
break;
+   }
LIMIT(j, 0, NUMFONTSCALES - 1);
drw_setfontset(d, fonts[j]);
 
@@ -381,7 +382,7 @@ cleanup(int slidesonly)
 void
 reload(const Arg *arg)
 {
-   FILE *fp = NULL;
+   FILE *fp;
unsigned int i;
 
if (!fname) {
@@ -414,15 +415,17 @@ load(FILE *fp)
/* read each line from fp and add it to the item list */
while (1) {
/* eat consecutive empty lines */
-   while ((p = fgets(buf, sizeof(buf), fp)))
+   while ((p = fgets(buf, sizeof(buf), fp))) {
if (strcmp(buf, "\n") != 0 && buf[0] != '#')
break;
+   }
if (!p)
break;
 
-   if ((slidecount+1) * sizeof(*slides) >= size)
+   if ((slidecount+1) * sizeof(*slides) >= size) {
if (!(slides = realloc(slides, (size += BUFSIZ
die("sent: Unable to reallocate %u bytes:", 
size);
+   }
 
/* read one slide */
maxlines = 0;
@@ -496,7 +499,7 @@ resize(int width, int height)
 }
 
 void
-run()
+run(void)
 {
XEvent ev;
 
@@ -518,7 +521,7 @@ run()
 }
 
 void
-xdraw()
+xdraw(void)
 {
unsigned int height, width, i;
Image *im = slides[idx].img;
@@ -528,15 +531,11 @@ xdraw()
 
if (!im) {
drw_rect(d, 0, 0, xw.w, xw.h, 1, 1);
-   for (i = 0; i < slides[idx].linecount; i++)
-   drw_text(d,
-(xw.w - width) / 2,
+   for (i = 0; i < slides[idx].linecount; i++) {
+   drw_text(d, (xw.w - width) / 2,
 (xw.h - height) / 2 + i * linespacing * 
d->fonts->h,
-width,
-d->fonts->h,
-0,
-slides[idx].lines[i],
-0);
+width, d->fonts->h, 0, slides[idx].lines[i], 
0);
+   }
drw_map(d, xw.win, 0, 0, xw.w, xw.h);
} else {
if (!(im->state & SCALED))
@@ -546,7 +545,7 @@ xdraw()
 }
 
 void
-xhints()
+xhints(void)
 {
XClassHint class = {.res_name = "sent", .res_class = "presenter"};
XWMHints wm = {.flags = InputHint, .input = True};
@@ -564,7 +563,7 @@ xhints()
 }
 
 void
-xinit()
+xinit(void)
 {
XTextProperty prop;
unsigned int i;
@@ -608,14 +607,13 @@ xinit()
 }
 
 void
-xloadfonts()
+xloadfonts(void)
 {
int i, j;
char *fstrs[LEN(fontfallbacks)];
 
-   for (j = 0; j < LEN(fontfallb

[hackers] [sent] [PATCH 1/3] sent.c: Drop unnecessary NULL checks

2022-06-26 Thread Tom Schwindl

Since no operation is performed when free(3) is used on NULL, remove
the checks.
---
 sent.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sent.c b/sent.c
index eb0ac970b1f5..3916cb0c4ad1 100644
--- a/sent.c
+++ b/sent.c
@@ -216,8 +216,7 @@ ffload(Slide *s)
s->img->bufwidth = ntohl(*(uint32_t *)&hdr[8]);
s->img->bufheight = ntohl(*(uint32_t *)&hdr[12]);
 
-   if (s->img->buf)
-   free(s->img->buf);
+   free(s->img->buf);
/* internally the image is stored in 888 format */
s->img->buf = ecalloc(s->img->bufwidth * s->img->bufheight, 
strlen("888"));
 
@@ -625,8 +624,7 @@ xloadfonts(void)
}
 
for (j = 0; j < LEN(fontfallbacks); j++)
-   if (fstrs[j])
-   free(fstrs[j]);
+   free(fstrs[j]);
 }
 
 void
-- 
2.36.1

[hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-26 Thread Tom Schwindl

Let the Makefile be a bit more verbose and remove unnecessary extensions
and flags in config.mk.
---
 Makefile  | 37 +++--
 config.mk | 18 --
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/Makefile b/Makefile
index 56e636734216..d73909283970 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,6 @@
 # sent - plain text presentation tool
 # See LICENSE file for copyright and license details.
+.POSIX:
 
 include config.mk
 
@@ -18,43 +19,43 @@ config.h:
cp config.def.h config.h
 
 .c.o:
-   @echo CC $<
-   @${CC} -c ${CFLAGS} $<
+   @echo ${CC} $<
+   ${CC} -c ${CFLAGS} $<
 
 ${OBJ}: config.h config.mk
 
 sent: ${OBJ}
-   @echo CC -o $@
-   @${CC} -o $@ ${OBJ} ${LDFLAGS}
+   @echo ${CC} -o $@
+   ${CC} -o $@ ${OBJ} ${LDFLAGS}
 
 cscope: ${SRC} config.h
@echo cScope
-   @cscope -R -b || echo cScope not installed
+   cscope -R -b || echo cScope not installed
 
 clean:
@echo cleaning
-   @rm -f sent ${OBJ} sent-${VERSION}.tar.gz
+   rm -f sent ${OBJ} sent-${VERSION}.tar.gz
 
 dist: clean
@echo creating dist tarball
-   @mkdir -p sent-${VERSION}
-   @cp -R LICENSE Makefile config.mk config.def.h ${SRC} sent-${VERSION}
-   @tar -cf sent-${VERSION}.tar sent-${VERSION}
-   @gzip sent-${VERSION}.tar
-   @rm -rf sent-${VERSION}
+   mkdir -p sent-${VERSION}
+   cp -R LICENSE Makefile config.mk config.def.h ${SRC} sent-${VERSION}
+   tar -cf sent-${VERSION}.tar sent-${VERSION}
+   gzip sent-${VERSION}.tar
+   rm -rf sent-${VERSION}
 
 install: all
@echo installing executable file to ${DESTDIR}${PREFIX}/bin
-   @mkdir -p ${DESTDIR}${PREFIX}/bin
-   @cp -f sent ${DESTDIR}${PREFIX}/bin
-   @chmod 755 ${DESTDIR}${PREFIX}/bin/sent
+   mkdir -p ${DESTDIR}${PREFIX}/bin
+   cp -f sent ${DESTDIR}${PREFIX}/bin
+   chmod 755 ${DESTDIR}${PREFIX}/bin/sent
@echo installing manual page to ${DESTDIR}${MANPREFIX}/man1
-   @mkdir -p ${DESTDIR}${MANPREFIX}/man1
-   @cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
-   @chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
+   mkdir -p ${DESTDIR}${MANPREFIX}/man1
+   cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
+   chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
 
 uninstall:
@echo removing executable file from ${DESTDIR}${PREFIX}/bin
-   @rm -f ${DESTDIR}${PREFIX}/bin/sent
+   rm -f ${DESTDIR}${PREFIX}/bin/sent
 
 .PHONY: all options clean dist install uninstall cscope
diff --git a/config.mk b/config.mk
index d61c55437f46..15fdcfa10dc7 100644
--- a/config.mk
+++ b/config.mk
@@ -11,20 +11,18 @@ X11INC = /usr/X11R6/include
 X11LIB = /usr/X11R6/lib
 
 # includes and libs
-INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
-LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
+INCS = -I/usr/include/freetype2 -I${X11INC}
+LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
 # OpenBSD (uncomment)
-#INCS = -I. -I${X11INC} -I${X11INC}/freetype2
+#INCS = -I${X11INC} -I${X11INC}/freetype2
 # FreeBSD (uncomment)
-#INCS = -I. -I/usr/local/include -I/usr/local/include/freetype2 -I${X11INC}
-#LIBS = -L/usr/local/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
+#INCS = -I/usr/local/include/freetype2 -I${X11INC}
+#LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
 
 # flags
 CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
-CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
-LDFLAGS += -g ${LIBS}
-#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
-#LDFLAGS += ${LIBS}
+CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes -Wold-style-definition 
-Os ${INCS} ${CPPFLAGS}
+LDFLAGS = ${LIBS}
 
 # compiler and linker
-CC ?= cc
+CC = cc
-- 
2.36.1

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

Re: [hackers] [sent] [PATCH 1/3] sent.c: Drop unnecessary NULL checks

Re: [hackers] [sent] [PATCH 1/3] sent.c: Drop unnecessary NULL checks

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

[hackers] [sent] [PATCH 2/3] treewide: Improve compliance with our coding style

[hackers] [sent] [PATCH 1/3] sent.c: Drop unnecessary NULL checks

[hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

7 matches

Site Navigation

Mail list logo

Footer information