* Re: [Edbrowse-dev] -Wall
2017-07-26 3:08 [Edbrowse-dev] -Wall Karl Dahlke
2017-07-26 4:45 ` Chris Brannon
@ 2017-07-27 18:21 ` Geoff McLane
2017-07-28 5:51 ` Karl Dahlke
1 sibling, 1 reply; 6+ messages in thread
From: Geoff McLane @ 2017-07-27 18:21 UTC (permalink / raw)
To: Karl Dahlke, Edbrowse-dev, Chris Brannon
Hi Karl, Chris,
Thanks for the heads up about the master
branch... with a few minor changes it
compiles and passes the jsrt test...
Re: Unix
Well the cmake build has always been adding
-Wall, but then suppressed some warning...
I have just removed those suppressions, as one
should do now and then to test, and it seems
there are no current warning...
Re: Windows
I guess the equivalent in Windows is the /W3,
which is added automatically by cmake...
But again I had been suppressing a number of
warnings...
There is a /W4, but do not suggest using this...
it seem /W3 is very sufficient...
Removing that suppression and MSVC14 2015 shows
736 warnings, no errors...
See full list -
http://geoffair.org/tmp/bldlog-w.txt
but be aware, each is listed twice. Once for the
Debug build, and once for the Release build...
One form of very common C4996 warning is this -
...\main.c(372): warning C4996: 'getenv': This function or
variable may be unsafe. Consider using _dupenv_s instead. To
disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online
help for details.
Most of the time I see no particular reason to adopt a
non-portable replacement, thus adding the suggested
macro _CRT_SECURE_NO_WARNINGS immediately reduces the
warnings to 393...
See http://geoffair.org/tmp/bldlog-w1.txt, but again
be aware of the Debug/Release duplicates...
But this leaves some other warning C4996, of the form -
...\main.c(492): warning C4996: 'close': The POSIX name
for this item is depreciated. Instead, use the ISO C and C++
conformant name: _close. See online help for details.
From discussion in several other project, it seems unix
people reject this MS claim... and I tend to agree...
It can be fixed by adding the following in a common
include file -
#ifdef _MSC_VER
#define close _close
#endif
Note this is just for a MS MSVC build, so use _MSC_VER
rather than the DOSLIKE macro you have...
But this is a list of functions like open, chmod, creat,
write, unlink, getcwd, etc...
Or suppressing them, as I have done in CMakeLists.txt
if (WIN32)
if (MSVC)
set(WARNING_FLAGS "${WARNING_FLAGS} /wd4996")
This reduces the warning to just 283. That is removed
some 453 warnings to date... see
http://geoffair.org/tmp/bldlog-w2.txt
The next most common warning is C4267 -
...\main.c(96): warning C4267: 'initializing': conversion
from 'size_t' to 'int', possible loss of data
This happen in the Windows 64-bit build because a size_t
is 8 bytes, while an int remains 4 bytes...
But in reality, an int is large enough to hold a
very big number, thus very very unlikely this would
represent loss of data, but could...
This can be fixed by telling the compiler you
know by a static cast, like in this case -
int rlen = (int)strlen(reply);
Or suppressing it -
if (WIN32)
if (MSVC)
foreach(warning 4267)
set(WARNING_FLAGS "${WARNING_FLAGS} /wd${warning}")
endforeach()
This reduced the warnings to 142. ie removed
some 141 more... see -
http://geoffair.org/tmp/bldlog-w3.txt
The next group of warnings are similar - suggest
possible loss of data, like -
..\main.c(176): warning C4244: '=': conversion from '__int64'
to 'int', possible loss of data
But some do raise suspicions, like -
..\main.c(1478): warning C4244: '=': conversion from 'long' to
'unsigned char', possible loss of data
The code here is -
td->key1 = strtol(v,&v, 10);
Now I have not studied the code carefully, but if
this represents for example a table td columns
then probably a maximum of 255 is ok...
But as you point out, sometimes fixing the code
with a static cast makes it clear to the compiler,
and to other readers of the code that you have
thought of this, and accept the risk...
And in this case the code following compares the
value to an 'int', td->ncols... why not make the
key1 and key2 ints also...
But for windows, added this 4244 to the suppression
list... this reduces the warnings to just 26 -
see http://geoffair.org/tmp/bldlog-w4.txt
The next most common is an unreferenced variable...
...\buffers.c(414): warning C4101: 'channels': unreferenced
local variable
Now usually gcc also warns of this, so this is
usually, like in this case, in a DOSLIKE macro
switch... so this, and other like cases could
be fixed by -
#ifndef DOSLIKE
fd_set channels;
#endif // !DOSLIKE
But I do not consider this bad coding when
there are macro switches involved, and maybe
the extra works to switch each of these is
not worth the effort...
So I have added 4101 to the suppression list...
this reduces the warnings to just 12 - see
http://geoffair.org/tmp/bldlog-w5.txt
Another minor item that MSVC flags is C4090,
like -
...\main.c(832): warning C4090: 'function': different
'const'
Again, I do not consider this important... it
could be 'fixed', but choose to just add
4090 to the suppression list...
And we are down to just 7 warnings - see
http://geoffair.org/tmp/bldlog-w6.txt
Five (5) of these are of the form -
...\sendmail.c(669): warning C4013: 'asprintf' undefined;
assuming extern returning int
Since this usually involves adding a header
to the source, I do not like suppressing these...
and since the link resolves them with no problem,
maybe they can stay...
One (1) is -
...\jseng-duk.c(954): warning C4018: '>': signed/unsigned
mismatch
Since this is in a new module, perhaps it could
be fixed, instead of suppression...
The last is -
..\vsprtf.c(24): warning C4005: 'va_copy': macro
redefinition
Since this is in a Win32 only module, and is
just due to a later version of _MSC_VER have
provided a fix...
Have pushed 4 commits addressing the above...
Regards,
Geoff.
^ permalink raw reply [flat|nested] 6+ messages in thread