Writing clean code isn't hard!

[PeterO]

Just to make a point... My current project....

Code: Select all

petero@HP173 ~/803-GTK3 $ make
Scanning dependencies of target 803
[  8%] Building C object CMakeFiles/803.dir/Emulator.c.o
[ 16%] Building C object CMakeFiles/803.dir/Contactor.c.o
[ 25%] Building C object CMakeFiles/803.dir/Keyboard.c.o
[ 33%] Building C object CMakeFiles/803.dir/Editor.c.o
[ 41%] Building C object CMakeFiles/803.dir/PTS.c.o
[ 50%] Building C object CMakeFiles/803.dir/Reader.c.o
[ 58%] Building C object CMakeFiles/803.dir/SimpleDrawers.c.o
[ 66%] Building C object CMakeFiles/803.dir/LoadInfo.c.o
[ 75%] Building C object CMakeFiles/803.dir/Hand.c.o
[ 83%] Building C object CMakeFiles/803.dir/Cursors.c.o
[ 91%] Building C object CMakeFiles/803.dir/Common.c.o
[100%] Building C object CMakeFiles/803.dir/fsm.c.o
Linking C executable 803
[100%] Built target 803

Code: Select all

petero@HP173 ~/803-GTK3 $ wc *.c 
   198    478   3679 Common.c
   107    190   2768 Contactor.c
   232    630   5106 Cursors.c
  1027   1949  26759 Editor.c
    41     69   1051 Empty.c
    96    213   2236 Emulator.c
    93    153   1569 fsm.c
   639   1273  14534 Hand.c
  1327   2923  34090 Keyboard.c
   441    847   9133 LoadInfo.c
   677   1234  16823 PTS.c
  1860   3932  43989 Reader.c
   588   1104  11817 SimpleDrawers.c
  7326  14995 173554 total
petero@HP173 ~/803-GTK3 $ 

That's Over 7000 lines of code compiled with -Wall -Wextra -Wpedantic with no warnings..... It can be done if you actually take time and care about what you are doing !
PeterO

[jahboater]

That's Over 7000 lines of code compiled with -Wall -Wextra -Wpedantic with no warnings..... It can be done if you actually take time and care about what you are doing !
PeterO

And its best done from the start, then its relatively easy.
I suggest adding -Wconversion to the minimum warning flags above.

I use this in the makefile for a "lint" target.

Code: Select all

WARN = -Wfatal-errors -Wall -Wextra -Wunused -Wconversion -Wundef -Wcast-qual \
       -Wredundant-decls -Wunreachable-code -Wwrite-strings -Warray-bounds    \
       -Wstrict-aliasing=3 -Wstrict-overflow=1 -Wstrict-prototypes -Winline   \
       -Wshadow -Wswitch -Wmissing-include-dirs -Woverlength-strings -Wpacked \
       -Wdisabled-optimization -Wmissing-prototypes -Wformat=2 -Winit-self    \
       -Wmissing-declarations -Wunused-parameter -Wlogical-op -Wuninitialized \
       -Wnested-externs -Wpointer-arith -Wdouble-promotion -Wunused-macros    \
       -Wcast-align -Wunsafe-loop-optimizations 
#  
#  Remove for older compilers
#  
WARN += -Wduplicated-cond -Wnull-dereference -Wshift-overflow=2

Note -O3 may produce more accurate messages (particularly uninitialised variables) due to the greater depth of analysis.

WARN could be used in a simple case thus:

Code: Select all

lint:
  $(CC) prog.c -O3 $(CFLAGS) $(WARN) $(LIBS) -o /dev/null

prog: prog.c
  $(CC) prog.c -Os $(CFLAGS) $(LIBS) -o prog

[PeterO]

That's Over 7000 lines of code compiled with -Wall -Wextra -Wpedantic with no warnings..... It can be done if you actually take time and care about what you are doing !
PeterO

And its best done from the start, then its relatively easy.
I suggest adding -Wconversion to the minimum warning flags above.

I use this in the makefile for a "lint" target.

Code: Select all

WARN = -Wfatal-errors -Wall -Wextra -Wunused -Wconversion -Wundef -Wcast-qual \
       -Wredundant-decls -Wunreachable-code -Wwrite-strings -Warray-bounds    \
       -Wstrict-aliasing=3 -Wstrict-overflow=1 -Wstrict-prototypes -Winline   \
       -Wshadow -Wswitch -Wmissing-include-dirs -Woverlength-strings -Wpacked \
       -Wdisabled-optimization -Wmissing-prototypes -Wformat=2 -Winit-self    \
       -Wmissing-declarations -Wunused-parameter -Wlogical-op -Wuninitialized \
       -Wnested-externs -Wpointer-arith -Wdouble-promotion -Wunused-macros    \
       -Wunsafe-loop-optimizations 
#  
#  Remove for older compilers
#  
WARN += -Wduplicated-cond -Wnull-dereference -Wshift-overflow=2

Note -O3 may produce more accurate messages (particularly uninitialised variables) due to the greater depth of analysis.

I find -Wconversion produces "noise" from my code. I'm quite happy to let the compiler silently deal with some implicit type conversions in my code. I guess I'm a bit "old school" in this respect.
For example, I'm quite happy to assign a float to an int when the float is the x coordinate from a mouse event. Next time I start a new file I'll add it

I'm not going to show you the compiler output with -Wconversion enabled because one of the things I'm doing in that project is converting one of the main structure's members from signed to unsigned ints and at the moment I've only tackled about half of the uses.

PeterO

[DougieLawson]

I use this in the makefile for a "lint" target.

Code: Select all

-Wfatal-errors -Wall -Wextra -Wunused -Wconversion -Wundef -Wcast-qual \
       -Wredundant-decls -Wunreachable-code -Wwrite-strings -Warray-bounds    \
       -Wstrict-aliasing=3 -Wstrict-overflow=1 -Wstrict-prototypes -Winline   \
       -Wshadow -Wswitch -Wmissing-include-dirs -Woverlength-strings -Wpacked \
       -Wdisabled-optimization -Wmissing-prototypes -Wformat=2 -Winit-self    \
       -Wmissing-declarations -Wunused-parameter -Wlogical-op -Wuninitialized \
       -Wnested-externs -Wpointer-arith -Wdouble-promotion -Wunused-macros    \
       -Wunsafe-loop-optimizations 

It would be useful is any folks who are providing code for beginners on this forum would compile their stuff with at least some of those flags. There's too much with invalid main() functions (returns void, does return int or simply uses return;) which isn't good for the folks who are new to C programming. If it's some junk example for something I'm trying for myself (before including it in a larger project) it probably doesn't matter. As soon as you publish it for other folks it should cleanly pass most of those compiler tests.

[jahboater]

I find -Wconversion produces "noise" from my code. I'm quite happy to let the compiler silently deal with some implicit type conversions in my code.

Fair enough.

For interest ...
It does "Warn for implicit conversions that may alter a value" (man gcc), benign conversions are silent.
int32_t n;
double x;
x = n;
will not raise a warning because the mantissa is 53 bits, bigger than an int.
but
int64_t n;
double x;
x = n;
will fail because something must be lost when a 64 bit integer is squeezed into a 53 bit mantissa.
More interestingly,
unsigned int a;
long b;
b = a;
will fail on the Pi in 32-bit mode because the sign could change, but succeed and not raise a warning on a 64-bit LP64 machine. You get different results on different platforms.

[jahboater]

-Wall/-Wextra surprisingly doesn't cover all that much, I don't know why more options are not included.
For example, daft code like:

static const char mystr[] = "asasdddad";
char *ptr = (char*)mystr;
*ptr = 'x';

will likely crash the program and I would always want to know about it (-Wcast-qual is needed).

[Paeryn]

-Wall/-Wextra surprisingly doesn't cover all that much, I don't know why more options are not included.
For example, daft code like:

static const char mystr[] = "asasdddad";
char *ptr = (char*)mystr;
*ptr = 'x';

will likely crash the program and I would always want to know about it (-Wcast-qual is needed).

Probably down to with C casting you are taking responsibility for what you are doing when you cast and there is only one way of explicit casting. Having the warning in the default sets could cause a lot of messages in lagacy code that uses it safely (i.e. doesn't attempt to write into the const).

[PeterO]

I've added your "long list" of warning switches to my CMakeList.txt file, and now I feel like a noob C programmer again
But I'm biting the bullet and will try to get back to a clean compile, but it might take me quite a while

PeterO

[PeterO]

I can't live with some of those warnings !! My code is careful to make sure functions come before their first use thus avoiding lots of pointless prototypes. I've had to disable the warnings that complain if functions are not prototyped even when they are defined before they are used.

PeterO

[jahboater]

I can't live with some of those warnings !! My code is careful to make sure functions come before their first use thus avoiding lots of pointless prototypes. I've had to disable the warnings that complain if functions are not prototyped even when they are defined before they are used.

Are you sure??? My code is the same as yours, I define functions before using them and don't bother with the prototypes where possible - and it does compile cleanly.

If you do get bored, I noticed that -fstrict-aliasing and -fstrict-overflow must be switched on (mine is in CFLAGS) before -Wstrict-aliasing and -Wstrict-overflow become active.

Yes, some warnings can be safely elided. For example -Wcast-align throws up a couple of annoying warnings where I have carefully and deliberately ensured the target alignment is correct, but the compiler doesn't know it.

[PeterO]

I can't live with some of those warnings !! My code is careful to make sure functions come before their first use thus avoiding lots of pointless prototypes. I've had to disable the warnings that complain if functions are not prototyped even when they are defined before they are used.

Are you sure???

Yes. -Wmissing-prototypes and -Wmissing-declarations both needed to be removed to get ride of this warning:

Code: Select all

/home/petero/803-GTK3/Editor.c:77:10: warning: no previous prototype for ‘transmitterIsEmpty’ [-Wmissing-prototypes]
 gboolean transmitterIsEmpty(void)

Adding a totally pointless prototype just before the actual function stops the warning message!
From gcc manual:

-Wmissing-prototypes (C and Objective-C only)

Warn if a global function is defined without a previous prototype declaration. This warning is issued even if the definition itself provides a prototype. Use this option to detect global functions that do not have a matching prototype declaration in a header file. This option is not valid for C++ because all function declarations provide prototypes and a non-matching declaration declares an overload rather than conflict with an earlier declaration. Use -Wmissing-declarations to detect missing declarations in C++.

-Wmissing-declarations

Warn if a global function is defined without a previous declaration. Do so even if the definition itself provides a prototype. Use this option to detect global functions that are not declared in header files. In C, no warnings are issued for functions with previous non-prototype declarations; use -Wmissing-prototypes to detect missing prototypes. In C++, no warnings are issued for function templates, or for inline functions, or for functions in anonymous namespaces.

I'm definitely not going to use them as they seem to warn against what is perfectly valid C ! I don't see how your code isn't falling foul of them like mine does.
PeterO

[jahboater]

Interesting!
What version of the compiler are you using?
Messages from gcc have improved quite a bit recently.

[jahboater]

I'm definitely not going to use them as they seem to warn against what is perfectly valid C ! I don't see how your code isn't falling foul of them like mine does.
PeterO

Neither do I (understand). I tried and its clean with gcc 4.9 and gcc 6.3. And its a larger program (over 15,000 lines).
I have also

Code: Select all

#  
#  GCC options
#
OPT += -fno-math-errno -fno-associative-math -fno-unsafe-math-optimizations \
       -fno-reciprocal-math -ffinite-math-only

OPT += -finline-functions -finline-small-functions -findirect-inlining \
       -fpartial-inlining -finline-functions-called-once -fearly-inlining

OPT += -funsigned-char -fwhole-program -fno-exceptions -fomit-frame-pointer       \
       -fstrict-aliasing -fstrict-overflow -ftabstop=4 -fexpensive-optimizations  \
       -fno-stack-protector -fno-unwind-tables -fno-asynchronous-unwind-tables    \
       -funsafe-loop-optimizations -foptimize-strlen -fmerge-all-constants -free  \
       -faggressive-loop-optimizations -fipa-pta -freg-struct-return

I wonder if -fwhole-program has anything to do with it.

[PeterO]

Interesting!
What version of the compiler are you using?
Messages from gcc have improved quite a bit recently.

4.6.3 on Linux Mint 17.3
4.9.2 on Pi
5.4.0 on Linux Mint 18

All seem to produce the same warnings in this situation.

PeterO

PS: -fwhole-program just breaks the link stage in a strange way !

[jahboater]

Well, at least you have done the investigation, proved its a false positive and can now safely remove those flags!

I guess its difficult, and perhaps too much to ask, but could you reproduce the problem in a small, self contained piece of code? If so, and it fails with gcc 6.3, I should perhaps submit it as a low priority bug to the gcc folks.

[PeterO]

Well, at least you have done the investigation, proved its a false positive and can now safely remove those flags!

I guess its difficult, and perhaps too much to ask, but could you reproduce the problem in a small, self contained piece of code? If so, and it fails with gcc 6.3, I should perhaps submit it as a low priority bug to the gcc folks.

I'll have a go (but it might be tomorrow evening before I get time to sort it out), As far as I can see the warning switches are doing what the manual says they should (see high lighted text in manual quote above). There must be another option in your set up that has some influence.

PeterO

[jahboater]

There must be another option in your set up that has some influence.

yes may well be.
Only bother with a test case if you are convinced its a bug, otherwise forget about it!

[PeterO]

I'm getting frustrated by lots of warnings about my mixing signed and unsigned values

For example libraries like Cairo and Gtk return signed values for the size of things like windows and wigets. But then things like memset want size_t for the size of things. I guess the trick is to minimise the number of casts
needed to convince the compiler that you do actually know what you are doing

PeterO

[joan]

I'll give this a miss and stick with -Wall. Reminds me of Ada programmers who assumed because a program clean compiled it must be correct.

[PeterO]

I'll give this a miss and stick with -Wall. Reminds me of Ada programmers who assumed because a program clean compiled it must be correct.

I tend to agree with sticking with -Wall and -Wextra, but then I also know I'm a bit behind the times when it comes to some aspects of modern C code. I've never taken the time to
update myself so this seems like an ideal opportunity to take a step back and spend some time catching up on a few things.

For example I'm sure that somewhere in 7000 lines of code there should be at least a few places where I should use "const"
PeterO

[jahboater]

and who said "Writing clean code isn't hard"

Declare everything const that doesn't change - then you will get lots more warnings!

.... but faster and most importantly, safer, code.

[PeterO]

and who said "Writing clean code isn't hard"

I'm well aware of the irony ! But there is "clean" and then there's "all shiny and clean". People that post code that isn't even clean with just "-Wall -Wextra" were the real target of this thread

PeterO

[jahboater]

People that post code that isn't even clean with just "-Wall -Wextra" were the real target of this thread

Recent threads have code posted that doesn't even compile!

[PeterO]

Recent threads have code posted that doesn't even compile!

Indeed, which wastes everyone's time !
PeterO

[Paeryn]

The -Wmissing-prototypes and -Wmissing-declarations warnings aren't meant as warnings against bad code, they are more for checking that external functions have declarations in header files. If you decided to rename a function in the source code of a library but forgot to rename it in the header file then the compiler would flag it up as the function would no longer have a matching declaration in the header. Just compiling the library wouldn't necessarily pick up on that if the function is never called from within the library itself.

missing-prototypes is for catching when you declare a function without giving a prototype. This will catch cases where you don't provide the function parameters in the header (which missing-declarations won't).