[Edbrowse-dev] -Wall

[Edbrowse-dev] -Wall

[Karl Dahlke]

Edbrowse now compiles no errors with -Wall flag, at least on my machine and on the web server.
Not sure if this changes very much, it made a few lines of code more readable I guess.
export EBSTRICT=1
and run make in the src directory.
This follows the model of EBDEBUG, but should we run with -Wall all the time,
(Geoff) what is the corresponding flag in windows and should we set it always or conditionally,
what changes should we make to CMakeLists.txt?
I have no strong feelings on any of this, so whatever you-all think.

Karl Dahlke

Re: [Edbrowse-dev] -Wall

[Chris Brannon]

Yes we should probably run with -Wall all the time.
Passing -O2 and -D_FORTIFY_SOURCE=2 in CFLAGS exposes more warnings.
Some of them should be taken very seriously.

-- Chris

Re: [Edbrowse-dev] -Wall

[Geoff McLane]

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.

[Edbrowse-dev] -Wall

[Karl Dahlke]

Thank you.

> ...\jseng-duk.c(954): warning C4018: '>': signed/unsigned

Fixed this with a cast.

Karl Dahlke

Re: [Edbrowse-dev] -Wall

[Adam Thompson]

On Tue, Jul 25, 2017 at 09:45:54PM -0700, Chris Brannon wrote:
> Yes we should probably run with -Wall all the time.
> Passing -O2 and -D_FORTIFY_SOURCE=2 in CFLAGS exposes more warnings.

I wonder if we shouldn't compile with these flags on by default also?  Just a thought, though I definitely think that we should enable all warnings by default and then explicitly suppress when we know we're ok.

Cheers,
Adam.

Re: [Edbrowse-dev] -Wall

[Geoff McLane]

Hi Karl,

Thanks for the cast fixes...

Pushed 2 small commits, now Windows is a 100% clean build...

Regards,
Geoff.