[Insight-developers] A massive number of new warnings

[Luis Ibanez]

Brad,

I'm all for treating Warnings as errors,
and agree with Dave's point that it is the way to
prevent them from being dismissed by developers.

In many cases they are hinting us to locate real errors.

For example, in this patch:

http://review.source.kitware.com/#change,679

The warnings in openjpeg were revealing bugs in the
API of many functions.

In particular, functions whose return type is declared
as UINT32, and the code of the implementations return
"-1", and code down the line compares against "-1".

http://review.source.kitware.com/#patch,sidebyside,679,1,Utilities/openjpeg/openjpeg.c

Those are real bugs,
that were hinted by warnings.


      Luis


----------------------------------------------------------------------------------------
On Thu, Jan 6, 2011 at 9:11 PM, Bradley Lowekamp <blowekamp at mail.nih.gov> wrote:
> Hello,
> I have a hard time getting excited about this issues when logic and valid
> code, which may be more optimal needs to be changes to suppress a warning
> only when -Wextra is enabled:
> http://review.source.kitware.com/#change,677
> There are useful warning and there are un-useful ones. At any rate, I agree
> that the dashboard needs some work to clean up some of these warnings. And I
> applaud the effort, and wish for more snow in New York to help get it done
> :)
> Brad
>
>
> On Jan 6, 2011, at 6:04 PM, David Cole wrote:
>
> On the dashboards it's not really an issue because they report warnings or
> errors, either one just fine.
>
> The problem is sitting in front of the keyboard... If it's not an error that
> prevents the build from completing, it will be ignored by *somebody*. And
> then, it just sneaks right in and forces those of us who monitor the
> dashboard health to take on the burden and the pain of cleaning it up. But
> if it is an error, then that same somebody has to deal with it before moving
> on: it'll get fixed much quicker if it's a build error for everybody who
> sees it.
>
> Of course, the other problem is that you can't build it yourself on *all*
> the platforms and different warnings show up on different platforms. So, in
> that respect, everybody causes a warning elsewhere once in a while through
> no fault of their own.
>
> But, just some food for thought: if "treat warnings as errors" were on by
> default for everybody's build, and hard to turn off, how many warnings would
> there be when compiling the source code. I'm guessing close to zero the vast
> majority of the time.
>
>
> ;-)
>
>
> On Thu, Jan 6, 2011 at 5:19 PM, Bill Lorensen <bill.lorensen at gmail.com>
> wrote:
>>
>> Eventually, if we can get warnings near zero, I agree that at least
>> for our dashboards we should set the flag that treats warnings as
>> error.
>>
>>
>> On Thu, Jan 6, 2011 at 4:13 PM, David Cole <david.cole at kitware.com> wrote:
>> > Actually the easiest way to get the point across (and win enemies and
>> > influence people) is to set the flag that treats warnings as errors.
>> >
>> > That'll grab their attention!!
>> >
>> > (Of course, you might not be able to get past the Utilities directory
>> > in the next decade, but.....)
>> >
>> >
>> > :-)
>> > David
>> >
>> >
>> > On Thu, Jan 6, 2011 at 4:02 PM, Hans Johnson <hans-johnson at uiowa.edu>
>> > wrote:
>> >> I was toying with the idea of adding something like is shown at the
>> >> bottom
>> >> of this message.
>> >>
>> >> We could safely build up a larger set of "recommended" compiler flags
>> >> for
>> >> each of the release types rather than just "-g" and "-O3".
>> >>
>> >> I think that -Wall should be the default flag, and that developers have
>> >> to
>> >> explicitly turn it off.  That way it's not just the 4 of us triaging
>> >> all
>> >> these complier warnings.
>> >>
>> >> NOTE:  The code below is not working, but I think it gets the point
>> >> across.
>> >>
>> >> Hans
>> >>
>> >>
>> >>
>> >>
>> >> #To create a portable build system, it is best to not test for
>> >> platforms,
>> >> but to test for features.
>> >> #
>> >> #Instead of testing "if Windows then do this", test for "if the
>> >> -Wno-invalid-offsetof flag works then use it". You can do that with the
>> >> CheckCCompilerFlag module, for example:
>> >>
>> >> include(CheckCCompilerFlag)
>> >> include(CheckCXXCompilerFlag)
>> >>
>> >> set(C_RELEASE_DESIRED_FLAGS "")
>> >> set(CXX_RELEASE_DESIRED_FLAGS "")
>> >>
>> >> # -Wall
>> >> check_c_compiler_flag(-Wall HAS_ALL_WARNINGS_C)
>> >> if (HAS_ALL_WARNINGS_C)
>> >>    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -Wall")
>> >> endif()
>> >>
>> >> check_cxx_compiler_flag(-Wall HAS_ALL_WARNINGS_CXX)
>> >> if (HAS_ALL_WARNINGS_CXX)
>> >>    set(CMAKE_CXX_FLAGS  "${CMAKE_C_FLAGS} -Wall")
>> >> endif()
>> >>
>> >> ## -pedantic
>> >> check_c_compiler_flag(-pedantic HAS_PEDANTIC_C)
>> >> if (HAS_PEDANTIC_C)
>> >>    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -pedantic")
>> >> endif()
>> >>
>> >> check_cxx_compiler_flag(-pedantic HAS_PEDANTIC_CXX)
>> >> if (HAS_PEDANTIC_CXX)
>> >>    set(CMAKE_CXX_FLAGS  "${CMAKE_C_FLAGS} -pedantic")
>> >> endif()
>> >>
>> >> ## -Wextra
>> >> check_c_compiler_flag(-Wextra HAS_EXTRA_WARNINGS_C)
>> >> if (HAS_EXTRA_WARNINGS_C)
>> >>    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -Wextra")
>> >> endif()
>> >>
>> >> check_cxx_compiler_flag(-Wextra HAS_EXTRA_WARNINGS_CXX)
>> >> if (HAS_EXTRA_WARNINGS_CXX)
>> >>    set(CMAKE_CXX_FLAGS  "${CMAKE_C_FLAGS} -Wextra")
>> >> endif()
>> >>
>> >> ## -Wshadow
>> >> check_c_compiler_flag(-Wshadow HAS_SHADOW_WARNINGS_C)
>> >> if (HAS_SHADOW_WARNINGS_C)
>> >>    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -Wshadow")
>> >> endif()
>> >>
>> >> check_cxx_compiler_flag(-Wshadow HAS_SHADOW_WARNINGS_CXX)
>> >> if (HAS_SHADOW_WARNINGS_CXX)
>> >>    set(CMAKE_CXX_FLAGS "${CMAKE_C_FLAGS} -Wshadow")
>> >> endif()
>> >>
>> >> ## -Wlong-long  We should not use the long-long type in ITK
>> >> check_c_compiler_flag(-Wlong-long HAS_LONG_LONG_WARNINGS_C)
>> >> if (HAS_LONG_LONG_WARNINGS_C)
>> >>    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wlong-long")
>> >> endif()
>> >>
>> >> check_cxx_compiler_flag(-Wlong-long HAS_LONG_LONG_WARNINGS_CXX)
>> >> if (HAS_LONG_LONG_WARNINGS_CXX)
>> >>    set(CMAKE_CXX_FLAGS "${CMAKE_C_FLAGS} -Wlong-long")
>> >> endif()
>> >>
>> >>
>> >> check_cxx_compiler_flag(-Wno-invalid-offsetof HAS_NO_INVALID_OFFSETOF)
>> >> if (HAS_NO_INVALID_OFFSETOF)
>> >>    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-invalid-offsetof")
>> >> endif()
>> >>
>> >> message(${CMAKE_C_FLAGS})
>> >> message(${CMAKE_CXX_FLAGS})
>> >>
>> >>
>> >>
>> >> On 1/6/11 2:55 PM, "Luis Ibanez" <luis.ibanez at kitware.com> wrote:
>> >>
>> >>> I just added -Wall and -Wextra to the builds in
>> >>> zion, thurmite and eldorado.
>> >>>
>> >>> It's time to follow Bill L. in the zero-warning campaign      :-)
>> >>>
>> >>>
>> >>>      Luis
>> >>>
>> >>>
>> >>> ----------------------------------
>> >>> On Wed, Jan 5, 2011 at 6:32 PM, Sean McBride <sean at rogue-research.com>
>> >>> wrote:
>> >>>> That build is only using -Wall and -Wextra, which my gcc dashboards
>> >>>> also
>> >>>> use, and yet mine show a much smaller number of warnings.  Strange
>> >>>> that
>> >>>> a newer gcc would not warn where an older gcc does, one reason could
>> >>>> be
>> >>>> that they were false positives in the older version.  In any case,
>> >>>> there
>> >>>> are less warnings than it seems since some are caused by headers and
>> >>>> so
>> >>>> repeated in many source files.
>> >>>>
>> >>>> On Wed, 5 Jan 2011 17:01:46 -0500, Bill Lorensen said:
>> >>>>
>> >>>>> I think they are valid warnings.
>> >>>>>
>> >>>>> On Wed, Jan 5, 2011 at 4:20 PM, Hans Johnson
>> >>>>> <hans-johnson at uiowa.edu> wrote:
>> >>>>>> This looks like the Insight-gcc-3.3. Has a definitive decision been
>> >>>>>> made
>> >>>>>> regarding compilers less than 4?  I thought that less than 3.4 was
>> >>>>>> excluded
>> >>>>>> because of incompatibility issues.
>> >>>>
>> >>>>
>> >>>>
>> >>
>> >> --
>> >> Hans J. Johnson, Ph.D.
>> >> Assistant Professor
>> >> 200 Hawkins Drive
>> >> T205 BT, The University of Iowa
>> >> Iowa City, IA 52242
>> >>
>> >> hans-johnson at uiowa.edu
>> >> PHONE: 319 353 8587
>> >>
>> >> _______________________________________________
>> >> Powered by www.kitware.com
>> >>
>> >> Visit other Kitware open-source projects at
>> >> http://www.kitware.com/opensource/opensource.html
>> >>
>> >> Kitware offers ITK Training Courses, for more information visit:
>> >> http://kitware.com/products/protraining.html
>> >>
>> >> Please keep messages on-topic and check the ITK FAQ at:
>> >> http://www.itk.org/Wiki/ITK_FAQ
>> >>
>> >> Follow this link to subscribe/unsubscribe:
>> >> http://www.itk.org/mailman/listinfo/insight-developers
>> >>
>> > _______________________________________________
>> > Powered by www.kitware.com
>> >
>> > Visit other Kitware open-source projects at
>> > http://www.kitware.com/opensource/opensource.html
>> >
>> > Kitware offers ITK Training Courses, for more information visit:
>> > http://kitware.com/products/protraining.html
>> >
>> > Please keep messages on-topic and check the ITK FAQ at:
>> > http://www.itk.org/Wiki/ITK_FAQ
>> >
>> > Follow this link to subscribe/unsubscribe:
>> > http://www.itk.org/mailman/listinfo/insight-developers
>> >
>
> <ATT00001..txt>
>