Re: [hackers] [st][PATCH 00/24] odg patches - fix warnings/errors, plug leaks, tidying up

From: <k0ga_AT_shike2.com>
Date: Tue, 22 Jan 2019 08:15:20 +0000

Hi,


> These patches fix all errors and warnings I found when building with lots of
> compiler warning flags enabled, and running tools like cppcheck and valgrind.
> There's lots of type and const correctness fixes, conflicting variable names,
> a couple of memory leaks, and stylistic stuff like reducing variable scope.
> Also avoid linking with libm and make building on OpenBSD a bit more
> straightforward.

First point, why do you think we care about what your linter says? If it
gives false positives send patches to the developers of your linter.

> Oliver Galvin (24):
> add gitignore

Why do you think we want a gitignore?. Suckless projects don't use
.gitignore files. If you want to ignore files you can do it locally
using .git/info/exclude

> avoid unnecessarily checking if an unsigned variable is < 0, fixes
> cppcheck warnings

I don't care about cppcheck. And you modified the macro ISCONTROL()
in a way that hides the difference beteween C0 control codes and C1
control codes. It is true that the code doesn't use this difference,
but it is additional documentation in the code.

> Reduce variable scope where possible, fix cppcheck style warnings

I don't care about cppcheck. Suckless projects always declare the
variable at the beginning of the function.

> use const where possible, avoid discarding const, fixes errors from
> -Wdiscarded-qualifiers

I don't care about cppcheck.

> avoid warnings from -Wunused-parameter

I don't care about cppcheck.

> fix warnings from -Wimplicit-fallthrough

I don't care about cppcheck. And you are modifying the behaviour of st!!!!.
How do you think that you can put random breaks and you are not going to
break the code?!?!?!?!?!?!?

> fix warning from -Wmaybe-uninitialized

I don't care about cppcheck.

> fix warnings from -Wsign-compare and -Wtype-limits. also make sure we
> use size_t for len variables

I don't care about cppcheck.

> avoid redundant declaration and old-style function definition

-extern char *argv0;
+static char *argv0;

Do you understand what you are doing there?!?!??!!?. You are creating
a different variable in every file including arg.h!!!!!!!.

> fix -Wshadow warnings, due to variable names conflicting with global
> variables. also we don't need to pass global variables to xinit()

I don't care about cppcheck.

> rename variable to fix cppcheck shadow warning, due to variable name
> conflicting with function

I don't care about cppcheck.

> fix remaining cppcheck warnings: reduce scope of some variables, and
> avoid compiling selcheck_ when it's not used, by adding a new
> setting in config.h

I don't care about cppcheck. Why do you think this ifdef is a good thing???
Because we move from 45KB to 44.9KB?

> clean up two warnings from clang about initialisation and sign
> comparison

I don't care about cppcheck. And now, I don't care about clang. Send
patches to clang to remove false positives.

> now st can build without errors/warnings with -Wall -Wextra -Wpedantic
> enabled, enable them by default

I don't care about cppcheck and I don't care about your warnings.

> update config.def.h with necessary changes after my previous commits.
> uses const everywhere and adds the SELCLEAR option

Ok, at this point I can confirm that you don't have too much experience
in programming, it is obvious. A free advise, don't hurt yourself and
don't use const.

> avoid leaking memory when xrealloc/xstrdup fail, by freeing memory
> before die()

I don't care about cppcheck and at this moment I don't care about valgrind.
There isn't any memory leak, all the memory of the process is freed at
the end of the process.

> use EXIT_SUCCESS/FAILURE

Please, don't submit style patches. St is a portable posix program, and in
the posix enviroment the exit status is properly defined and the use of 0
and 1 is correct.

> use compiler attribute to check die() parameters, and fix relevant
> warning from clang

Over my dead body. Learn to use C, please don't use GNU extensions. At this
moment st can be compiled with any c99 compliant compiler: gcc, clang, pcc,
lcc, tcc ...

> improve type and const correctness by using more correct types. fixes
> a bunch of -Wcast-qual warnings

Your patch is not correct, rejected.

> use EXIT_FAILURE/SUCCESS on exit

At least you should learn to squash commits before sending them to any
open source project. That's a pity that the mail history cannot be
rewritten. Any recruiter that will search for your name in the future
will discover this shame of patchset. Second free advise, before
doing things try to learn from more experienced guys.

> avoid unnecessarily linking to the math library, and detect openbsd
> automatically in makefile

Over my dead body!!!!!!!, what the fuck are you thinking???. Only the part
of removing -m is good. You should learn to use POSIX Makefiles. At this
moment st can be compiled using any POSIX Make: GNU make, BSD make, Solaris
make, AIX make ...

> fix memory leak - destroy patterns in xloadfont

Uhmmm, I have to take a deeper look to this patch, it seems correct.
Congratulation, this patch makes a bit of sense!

> stylistic stuff: Tidy up indentation, small fixes to comments, etc.

We don't accept style changes.

> link with librt on non-openbsd systems, I forgot to uncomment

Learn to rebase.


Kind regards,
Received on Tue Jan 22 2019 - 09:15:20 CET