Windows build warnings in mapgd.c and others

[hobu]

Folks,

My nightly windows build process was showing 80+ warnings in mapgd.c when
building with MSVC.  I took some time yesterday to clean up these warnings.  As
you can see in this build output
<http://hobu.stat.iastate.edu/mapserver/build_output/9_26_2004/mapserver_full.txt>,
most of the warnings were related to doubles and floats being automatically cast
into integers (MSVC appears to expect you to be explicit to suppress the warning).  

If someone gets some time, please look at this diff
<http://hobu.stat.iastate.edu/mapserver/build_output/9_28_2004/mapgd.c.diff> to
see if the choices for the casts that I made are appropriate.  After compiling
this, all of Sean's tests still pass, and I didn't notice any changes in GD's
rendering, but I would like someone else to help me review this before checking
it in as it might cause some very subtle errors if the wrong cast was chosen.

As for warnings in other files, I think it would be a good goal (but probably a
low priority, unfortunately) to have a warning-free build in Windows by the time
4.4 is released.  If you have a chance to take a look at the build output and
can quickly patch things up to suppress the warning, it would be much
appreciated.  Here is the current status of number of warnings.  Nightly builds
are located at <http://hobu.stat.iastate.edu/mapserver/build_output> to see the
effect of your changes on the build.

Howard

MSVC warnings in a full build (PDF, SWF, SDE, PostGIS, Oracle and others) --
9/28/2004
-------------------------------------------------------------------------------------------------
File			Warnings	Notes
-------------------------------------------------------------------------------------------------
cgiutil.c		3		< and > mismatches/undefined functions
mapdraw.c		2		undefined functions/unused variable
mapdrawgdal.c	3		Integer casts
mapfile.c		4		Integer casts
mapimagemap.c	11		const issues/Integer casts
maplabel.c		4		< and > mismatches/argument mismatch
maplayer.c		1		undefined functions
maplegend.c		2		Integer cast/argument mismatch
maporaclespatial.c	2		signed/unsigned mismatch
mappdf.c		6		Integer casts/unused variable/pointer indirection
mappostgis.c		4		argument mismatch/function without a return value
mapprojhack.c		1		Inconsistent DLL linkage
maprasterquery.c	1		Integer casts
mapresample.c	4		Integer casts
mapsde.c		9		argument mismatch
mapserv.c		8		Integer casts
mapshape.c		2		Integer casts
mapstring.c		1		< and > mismatches
mapswf.c		62		Integer casts/incompatible types/functions with too many parameters
mapsymbol.c		4		Integer casts
maptime.c		2		undefined functions/pointer indirection
mapwms.c		1		undefined functions
mapxbase.c		4		< and > mismatches
sortshp.c		5		undefined functions/function call issues

[fwarmerdam]

I have corrected a bunch that relate to me, and some that don't.  What warning
level did you run at?  I did my work with /W3. 

[hobu]

The makefile I am using is located at
http://hobu.stat.iastate.edu/mapserver/build_output/Makefile_full.vc 

It doesn't appear as though there is any warning level set, so I would say the
default ;)  Who knows what that means for Microsoft though.

I commited my fixes to mapgd.c tonight.  Hopefully, they won't cause too much
trouble (famous last words)...

[fwarmerdam]

I fixed a few additional odds and ends.   I have fixed for mappostgis.c pending
but they are mixed with connection pooling changes so I have not committed
them. 

I have verified that the default warning level is /W3 (in nmake.opt).  I tried 
/W4 but it produces alot of junk warnings. 

[hobu]

I cleaned up the integer casts on mapfile.c and maplabel.c  It's starting to get
pretty clean!

I also confirmed that my build is using /W3

[hobu]

cleaned up mapsde.c

[dmorissette]

Guys, unfortunately yesterday I didn't have time to look in details at the
changes in mapgd.c, but from a quick browse of the diffs I was wondering if the
best fix in some cases would not have been to change the type of some variables
from double to int instead of type-casting. (Once again I didn't look at the
code, just the diffs)

Of course type-casting avoids the warning, but the purpose of the warnings is to
get you to write cleaner code, so if you just plug type casts everywhere, you
may feel better when you type make, but your code is actually messier.

Food for thought anyway.

[hobu]

Daniel,

Good point about feeling better about the build.  That is also the reason why I
wanted someone to look over things and suggest what should be done.  I didn't
want in any way to materially change how mapgd.c worked because I know so little
about it.  I was hoping someone would see what I did and correct me, but no one
piped up about it.

When I was putting in all of the explict casts, my thought process was something
like this: "This is stupid, I should just change the type. Maybe everything is
supposed to be in ints because we're dealing with pixel values and it doesn't
make sense to have 0.34 of a pixel."  

However, I also didn't want to break anything since I don't know mapgd.c's
history and I'm not that familiar with how the rendering of MapServer works.  If
someone with more familiarity of mapgd.c has time back out my changes and do
them up right, I think it would be great and give folks like me a chance to
learn how one should go about doing those kinds of things in the future.  

As for having a build without warnings, whether it is actually rational or not, 
in my opinion, it does add to the professionalism of the software.  It also
gives folks one less reason to complain about or question something, and all of
that warning noise can obscure problems when things *really* change.  

Another data point is that most people are *very* aware of warnings and errors
on the unix-like platforms because MapServer is built daily there by many
different people.  Windows has kind of been neglected because so few people
actually build their own (although I'm sure there are lots of Windows MapServer
users).  GDAL, libpq, and many others build on MSVC without warnings, and IMO,
it's not unreasonable to have MapServer build cleanly on this platform as well.  

Again, this is not a jihad for me.  More of a "gee wouldn't it be nice" and my
willingness to put some effort into it.  

still learning....

Howard

[dmorissette]

I think it's worth taking the time to properly fix the warnings (and I'm
supporting you in your venture for a warning-free build). As you wrote, you may
not be comfortable changing mapgd.c, and I would not be comfortable changing
Frank's mapresample.c either, so I think it's up to the owner of each file to
fix their warnings properly.

The big question is how to make that happen when nobody except Frank replied to
your call to fix warnings?  I for oneapologize, I'll try to walk through the
files that still haven't been fixed, and if in doubt on any file I will file
separate bugs for specific files to discuss the issues and assign to the module
owner. I will probably also try to see if anything can be improved in mapgd.c...
unfortunately once again all this takes time... time is always the problem.

[dmorissette]

We've had to rollback the changes that Howard did to mapgd.c r1.65 because that
had introduced at least one bug (bug 923). 

We'll need someone who knows mapgd.c better (Steve, Frank, maybe me?) to walk
through the code and fix the warnings and make sure that doesn't break anything.

[sdlime]

If someone can send warning locations I'll take care of the GD code. Howard 
posted some diffs on the dev list and I can use those too.

Steve

[hobu]

Reviving this for the 4.10 release.  Here's the current warnings with CVS HEAD:

mapgd.c(1074) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1074) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1079) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1079) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1082) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1082) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1332) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1332) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1585) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1586) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(1590) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(2089) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(2089) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(3134) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(3135) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(3185) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data
mapgd.c(3185) : warning C4244: 'function' : conversion from 'double' to 'int',
possible loss of data

[hobu]

cleaned these up by casting them to unsigned char*'s like the function expects.

cleaned up a few more in mapxbase.c, and I commented out a statement that wasn't
doing anything/being used in maplayer.c.  Expect some more soon.

[hobu]

currently the buildbot only has 1 warning each in both the linux and osx slaves.
 On the linux one, the strptime prototype isn't being found.  On the OSX one
(which also builds iconv, unlike the linux one) complains about the 2 argument
being passed in to the function on line 2893 being an incompatible pointer type.
 A clean-building MapServer is a real (and quite scary :) possibility for 4.10.

http://mapserver.builds.hobu.net

[dmorissette]

Howard, I believe you fixed the last warnings, correct? If that's not the case
then I think we should set the target milestone to 5.0

[hobu]

There's still a few, but we're much improved.  I'll kick this along to 5.0

Howard

[hobu]

I'm only seeing one warning in maptime.c which I filed as a separate bug #2226

Closing this bug to pad our statistics :)