[mapserver-dev] Mixed code and variable definition breaks MSVC builds

[thomas bonfort]

On Sun, Jul 22, 2012 at 12:21 AM, Even Rouault
<even.rouault at mines-paris.org> wrote:
> Le samedi 21 juillet 2012 23:41:20, Tamas Szekeres a écrit :
>> Dear Committers,
>>
>> I usually find issues in the mapserver codebase due to the mixed code and
>> variable definitions committed into the code according to the C++/C99 style
>> coding convention. This makes the code uncompilable with MSVC which is
>> still pretty much C89 with a few minor extensions, and it doesn't support
>> this practice by default. We could definitely rename the corresponding c
>> files to cpp or force the c++ compilation or we can use a commandine option
>> to provide the same effect if we would like to follow this convention.
>>
>> In the past I continouosly fixed this issue by modifying the corresponding
>> code to use C89 forward definitions, but I'm not sure I'd like to take the
>> responsibility do such things in a daily practice. At the moment we also
>> have such issues in mapserver/master breaking the Windows builds for
>> several days.
>>
>> What would be the reasonable solution to prevent this issue happening so
>> frequently? We should probably make a decision whether we should compile
>> MapServer with MSVC for C++  or declare we might want to use forward
>> definitions everywhere in the code (Probably MS RFC 7.1 should be modified
>> with this).
>>
>> BTW: gcc users may sometimes take a look at the daily builds at
>> http://www.gisinternals.com/sdk/ to make sure thinks are working correctly
>> with MSVC or not.
>
> Based on what exists in GDAL, I suggest the following change in configure.in,
> as well as the addition of m4/ax_cflags_warn_all.m4, which will add by default
> the "-Wdeclaration-after-statement" flag in CFLAGS with GCC, which generates
> "warning: ISO C90 forbids mixed declarations and code" (what would be cool
> would be to error out, only on that error, instead of just warning)

+1 for adding the warning
+1 to make it an error if that is possible

--
thomas

>
> maputil.c: In function 'msOffsetPolyline':
> maputil.c:1748: warning: ISO C90 forbids mixed declarations and code
> maplabel.c: In function 'msAddLabelGroup':
> maplabel.c:361: warning: ISO C90 forbids mixed declarations and code
> maplabel.c: In function 'msAddLabel':
> maplabel.c:552: warning: ISO C90 forbids mixed declarations and code
> maplabel.c:576: warning: ISO C90 forbids mixed declarations and code
> mapresample.c: In function 'msNearestRasterResampler':
> mapresample.c:119: warning: unused variable 'nValue'
> mapresample.c: In function 'msSourceSample':
> mapresample.c:272: warning: ISO C90 forbids mixed declarations and code
> mapresample.c: In function 'msBilinearRasterResampler':
> mapresample.c:445: warning: ISO C90 forbids mixed declarations and code
> mapdraw.c: In function 'msDrawVectorLayer':
> mapdraw.c:1043: warning: ISO C90 forbids mixed declarations and code
>
>
> diff --git a/configure.in b/configure.in
> index 92c94b5..304b7b7 100755
> --- a/configure.in
> +++ b/configure.in
> @@ -36,6 +36,7 @@ m4_include([m4/mapserver.m4])
>  m4_include([m4/apache.m4])
>  m4_include([m4/ax_jni_include_dir.m4])
>  m4_include([m4/ax_pkg_swig.m4])
> +m4_include([m4/ax_cflags_warn_all.m4])
>
>  AC_CONFIG_SRCDIR([mapserv.c])
>  dnl Checks for programs.
> @@ -2162,42 +2163,33 @@ AC_MSG_CHECKING(compiler warnings)
>  AC_ARG_WITH(warnings,
>  [  --with-warnings[[=flags]] Enable strict warnings (or user defined
> warnings)],,)
>
> +AX_CFLAGS_WARN_ALL(C_WFLAGS)
> +AX_CXXFLAGS_WARN_ALL(CXX_WFLAGS)
> +
>  if test "$with_warnings" = "yes" ; then
>    if test "$GCC" = "yes" ; then
> -    WFLAGS="-W -Wall -Wcast-align -Wmissing-prototypes -Wstrict-prototypes -
> Wpointer-arith -Wreturn-type"
> -    C_EXTRA_WFLAGS="-Wmissing-declarations"
> +    WFLAGS="-W -Wcast-align -Wmissing-prototypes -Wstrict-prototypes -
> Wpointer-arith -Wreturn-type"
> +    C_WFLAGS="$C_WFLAGS -Wmissing-declarations"
>
>      AC_MSG_RESULT([strict])
>    else
>      WFLAGS=""
> -    C_EXTRA_WFLAGS=""
> -    AC_MSG_RESULT([disabled])
> -  fi
> -
> -elif test "$with_warnings" = "" ; then
> -  if test "$GCC" = "yes" ; then
> -    WFLAGS="-Wall"
> -    C_EXTRA_WFLAGS=""
> -    AC_MSG_RESULT([basic])
> -  else
> -    WFLAGS=""
> -    C_EXTRA_WFLAGS=""
>      AC_MSG_RESULT([disabled])
>    fi
>
>  elif test "$with_warnings" = "no" ; then
>    WFLAGS=""
> -  C_EXTRA_WFLAGS=""
> +  C_WFLAGS=""
> +  CXX_WFLAGS=""
>    AC_MSG_RESULT([disabled])
>
> -else
> +elif test "$with_warnings" != "" ; then
>    WFLAGS="$with_warnings"
> -  C_EXTRA_WFLAGS=""
>    AC_MSG_RESULT([user defined])
>  fi
>
> -CFLAGS="$CFLAGS $WFLAGS $C_EXTRA_WFLAGS"
> -CXXFLAGS="$CXXFLAGS $WFLAGS "
> +CFLAGS="$CFLAGS $WFLAGS $C_WFLAGS"
> +CXXFLAGS="$CXXFLAGS $WFLAGS $CXX_WFLAGS"
>
>  dnl ---------------------------------------------------------------------
>  dnl Check --enable-debug option for "-g" compile flag. (OFF by default)
>
>>
>> Best regards,
>>
>> Tamas
> _______________________________________________
> mapserver-dev mailing list
> mapserver-dev at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapserver-dev